Hi,
I had a first look at anonym's work on the feature/tor-browser-bundle
branch. Here are a few initial comments.
> +Explanation: Block installation of iceweasel until it has been removed from our APT repo
> +Package: iceweasel
> +Pin: origin deb.tails.boum.org
> +Pin-Priority: -1
I'm curious why we need that at all, and the explanation isn't very
convincing: even once we remove it from our own APT repo, it will be
available in the Debian ones, so I don't get it.
Regarding config/chroot_local-hooks/10-tbb, a lot of the code could
enjoy some refactoring. Currently, configuration, low-level processing
and the high-level flow are too strongly intermingled for my taste.
> +TBB_EXT="${TBB_INSTALL}/extensions"
I'm curious why we need to put extensions in a custom place, instead
of letting them live in the place as in the TBB.
> +BUNDLES="$(cat <<EOF
> [...]
> +EOF
> +)"
I'm not sure why this wouldn't work:
BUNDLES="
[...]
"
> +mkdir -p "${TBB_INSTALL}" "${TBB_PROFILE}" "${TBB_EXT}"
Here, we don't implicitly rely on the fact $TBB_EXT is a subdir of
$TBB_INSTALL, while the chown/chmod at the end of the script does.
Please pick one (probably the slightly safer explicit version) to
avoid potentially hard-to-debug future problems :)
> + APT_PROXY=$(sed -n 's/Acquire::http::Proxy "\(.*\)";/\1/p' /etc/apt/apt.conf.d/00http-proxy)
Instead, I would use something like:
apt-config --format '%v' dump Acquire::http::Proxy
> + curl -O "${TBB_DIST_URL}/${tarball}"
Please use long option names to ease reviewing, auditing and
future maintenance.
> +# We'll use the en-US bundle as our basis ...
Spurious space before "...".
> +TBB_PREP="${TMP}"/tor-browser_en-US
After extracting the bundle, I would explicitly check that this
directory exists, to make things clearer if the top-level directory
shipped in the tarball is renamed some day.
> +tar -xJf "${TMP}/${MAIN_BUNDLE}" -C "${TMP}"
and
> + tar xJf "${tarball}" "${xpi}"
Two remarks here:
* Better not use "J", as any not-too-old tar auto-detects compression
anyway, and thus there's no need to encode the current compression
algorithm in our code.
* Please pick one between "-xf" and "xf", and use it consistently.
> +# We don't want tor-launcher to be part of the browser, and we need our
> +# patched stand-alone version any way.
s/and/as/ ? Also, I suggest pointing to the parent ticket that tracks
upstreaming our changes.
> +# Remove TBB's torbutton since the "Tor test" will fail and about:tor
> +# will report an error. We'll install our own Torbutton later, which
> +# has the extensions.torbutton.test_enabled boolean pref as a workaround.
Same here, please point to the relevant ticket.
I would do all the iceweasel equivs things in a tempdir: if some day
equivs creates other files than the one we're manually cleaning up,
then the current code will happily put these files into /root in
the SquashFS.
> +ln -s /usr/share/xul-ext/adblock-plus/ "${TBB_EXT}"/'{d10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d}'
> +ln -s /usr/share/xul-ext/foxyproxy-standard/ "${TBB_EXT}"/foxyproxy@???
> +ln -s /usr/share/xul-ext/torbutton/ "${TBB_EXT}"/torbutton@???
It would be nice to take 15 minutes to try and extract the right-hand
values dynamically. If it's not done after 15 minutes, give up :)
> +cp -r "${TBB_PREP}"/Data/Browser/profile.default/* "${TBB_PROFILE}"/
This doesn't copy "hidden" files, right? I would use rsync instead,
and try to preserve more inode information.
> +rm -rf "${TMP}"
If `rm -r' works, better avoid the -f that will hide errors from `set -e'.
In config/chroot_local-hooks/12-remove_unwanted_browser_searchplugins:
> +PLUGIN_DIR=/usr/local/lib/tor-browser/Browser/browser/searchplugins
It seems that we're hard-coding the same path information in different
places. How about setting TBB_INSTALL and friends in a common place,
that can be sourced by all scripts that need it?
> + sed -i '/^user_pref("extensions.torlauncher.prompt_at_startup"/d' "${pref}"
I think that the dots might need to be escaped (and if there's
a problem here, yes, it was already in the previous code), but my sed
regexp foo is pretty low (I usually switch to Perl as soon as I'm
unsure).
> - daddr 127.0.0.1 proto tcp syn mod multiport destination-ports (9050 9061 9062 9151) {
> + daddr 127.0.0.1 proto tcp syn mod multiport destination-ports (9050 9061 9062 9150) {
It would be nice for anyone who has custom configuration that depends
on the SOCKS port to keep the 9151 we've been using until now. OTOH,
maybe people are installing scripts from the Internet that rely on the
default TBB port, so I'm unsure what's best. Is there a good reason to
change it?
> +cp -r /etc/tor-browser/profile "${USER_PROFILE}"/profile.default
I would use rsync and preserve more inode information.
> +# Use the Tor Browser's hardcoded relative-patch only default profile
s/patch/patch/ ?
> + mkdir -p /usr/Data/Browser/profile.default/preferences
/usr/Data/, really? Is that the "hardcoded relative-patch only default
profile dir" mentioned above? Maybe a symlink would allow to put the
actual files in a more relevant place, while keeping compatibility
with the TBB's code?
> +Comment=Anonymous Web Browser
Spurious trailing white-space.
> +apt-get -o Dpkg::Options::="--force-confdef" -o Dpkg::Options::="--force-confold" -y -t wheezy-backports install apt-cacher-ng
It's unclear to me, without reading the doc, how these apparently
conflicting dpkg options play together. Maybe add a comment?
Great work!
Cheers,
--
intrigeri
