[pkg] backdoor-factory ready for review

[phil at reseau-libre.net]

On 2017-06-22 18:14, Raphael Hertzog wrote:
> Hello Philippe,
> 
Re-hello Raphaël,

> 
> Comments and questions as I get them:
> 
> - the biggest problem is the installation in dist-packages with 
> setup.py
>   and pybuild, you are polluting the Python namespaces with many 
> generic
>   top-level packages such as "arm", "intel", "elfbin", "winapi",
>   "preprocessor", etc. This is not acceptable. There's a reason why we
>   installed it in a private directory in Kali. ;-)
>   You should install in dist-packages only when upstream supports it 
> and
>   when he has namespaced everything under a package that he clearly
>   "owns" (because it reuses the name of his software for example).

I've updated the packaging model by defining a "bdfactory" package. The 
various modules are installed as modules of the bdfactory package so 
that everything is in this directory in the dist-packages directory. 
This require some patch in the .py files on the import line but this 
permit to install properly in the dist-packages dir instead of 
/usr/share.
I've tested the tool in a sid chroot, it works properly with the new 
install. I've executed the debian-ci scripts to check.

Before posting an issue (and a merge request with the patch), i would 
like to have your point of view on this. If you think it is ok, i will 
create the issue & merge request on upstream and add the Bug: line in 
the patch header.

> 
> - please switch to Files-Excluded in debian/copyright instead of using
>   debian/orig-tar.sh as uscan hook script

Corrected.

> 
> - in debian/tests/control you have dependencies on "mktemp" which are 
> not
>   needed, it's provided by coreutils which is essential
> 
ok. Moreover the usage of $AUTOPKGTEST_TMP make it useless...

> - in your tests, you are creating temporary directories but you don't 
> have
>   to do that, you can just use "$AUTOPKGTEST_TMP" which is a temporary
>   directory created for you by autopkgtest, if needed you can create
>   arbitrarily-named sub-directories there obviously.

updated to use it instead.

> 
> - in your tests, you use full path "/usr/bin/backdoor-factory" but I
>   assume that you can rely on PATH being set to a reasonable value 
> (which
>   includes /usr/bin/)

Yes there is no real reason i've done that. Updated.

> 
> - as you seem to open arbitrary TCP ports, you probably need to add
>   "isolation-container" in the restriction.

Added.

> 
> - you don't need "Testsuite: autopkgtest" in debian/control, 
> dpkg-source
>   will add it for you in the .dsc
> 
> - "XS-Python-Version: >= 2.7" is not very useful (cf previous review)
> 

Corrected. As Std-version updated to 4.0.0.

> - use "Priority: optional"

Corrected too.

> 
> - you don't need to hardcode the dependency on "python", it's added by
>   ${python:Depends} already.

ack.

> 
> - note that in general, you want to put module dependencies in
>   Build-Depends as well so that you can run tests when you have some 
> (here
>   we don't)
> 
> - drop debian/dirs it's useless (directories are created on-demand by
>   dh_install)

ok done.

> 
> - rename debian/docs into debian/backdoor-factory.docs and drop 
> "debian/*"
>   from that file... README.Debian is automatically installed,
>   README.source is meant for packagers only and should not be installed
>   in binary packages.

done. I'm asking myself if this file is still usefull as, if i remember 
well, the README.md file is automatically added by dh_installdocs. This 
file is the last content.

> 
> - let's bump standards-version to 4.0.0 now on all freshly updated
>   packages

done.

> 
> - right now you are installing debian/helper-script/backdoor-factory
>   in /usr/bin/ (via dh_install) and just afterwards you are overwriting
>   that file with the content of backdoor.py
>   => this will be a non-issue if you switch back to a private directory
>   instalation as then you will have to rely on Python searching for
>   modules in the directory of the script being run.

Updated (cleaned helper script to install the script directly in 
/usr/bin. I didn't find a way to name the script properly (without the 
.py extention) so there is a line in the rules file to rename it. I 
don't know which is the better way beetween installing the utility 
directly in /usr/bin or using a helper script yet i don't see the 
advantage of using such an indirection (in the case of dist installed 
package).

> 
> - you don't need to pass "-O--buildsystem=pybuild" yourself when you
>   override some debhelper targets
> 

Updated.

> - you have those two useless lines in debian/rules (BTW DEB_TARGET_ARCH 
> is
>   probably not what you think it is, it's only involved when building
>   cross-compilers IIRC)
> # testing target arch.
> export DEB_TARGET_ARCH = $(shell dpkg-architecture -qDEB_TARGET_ARCH)

Yes deleted, should be a copy/paste from a compiled software which 
install a lib in /lib/<arch_os>/... Since that i've found in the debian 
docs the proper way to handle it, through a specific dpkg-architecture 
variable. This is not needed here.


> 
> - I would like you to discuss with upstream for the changes in
>   debian/patches/output_file_target.patch => more control on the
>   output files is certainly welcome in the general case
> 
>   At the same time, outputting a message about Debian not allowing
>   something looks wrong. We might be missing "appack" but the user 
> might
>   have installed it in some other way. You can fail gracefully if you
>   don't have it without claiming that Debian doesn't support it.

Ok I've updated so that if the appack bin exists, the code is executed, 
otherwhise, a message only print that appack is not found.
The indent update make the patch bigger but there is less lines really 
modified.
The output dir part of the patch (other that Debian related due to 
appack check) has been pushed as an issue in the upstream and the patch 
file reference it.

> 
>   BTW the correct test is "self.OUTPUT is None" and not "==" IIRC.

Corrected.

>   And assigning « self.backdoorfile = "backdoored/" + self.OUTPUT »
>   when you have checked that self.OUTPUT is None looks like useless
>   since you know that self.OUTPUT doesn't contain anything.

Corrected.

> 
>   => in the current state, this patch is not good at all
> 
> Cheers,

-- 
Philippe THIERRY
Doctor - Engineer
RT and hardened Embedded Systems
+33(0)6.64.16.97.30