19/09/14 01:08, intrigeri wrote:
> Hi,
>
> I had a first look at anonym's work on the feature/tor-browser-bundle
> branch.
I don't want to block your and kytv's work, so I think I'll just leave
the behemoth commit as-is, but with a more informative commit message.
Force pushed. Unless you have an issue with this, you may start basing
work on this branch now.
I'll fix the automated tests on Monday. Oh, and if you've noticed that
our bookmarks doesn't work, I'm already aware of it. I had the intention
to create tickets about what's remaining, but time flied, and I'm afraid
that too will have to wait until Monday.
> 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.
I'm not exactly sure why this is needed but without it our iceweasel
package is pulled when an `apt-get dist-upgrade` is issued (which e.g.
live-build does some time after the hooks). However, Debian's package
isn't pulled. I suppose the Pin-Priority of the fake Iceweasel package
installed (with `dpkg -i`) is lower than deb.t.b.o's 1005, but higher
than the 990 we assign to the Debian repos.
> 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.
Could you please elaborate?
>> +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.
I.e. directly in the browser profile skeleton at /etc/icewease/profile?
Well, I'd like to just be able to copy the profile skeleton when
creating a new profile without wasting space (well, RAM because tmpfs)
on duplicating every extension.
>> +BUNDLES="$(cat <<EOF
>> [...]
>> +EOF
>> +)"
>
> I'm not sure why this wouldn't work:
>
> BUNDLES="
> [...]
> "
The latter would break the read-while-loop later on since the first
iteration would read an empty line. And beginning listing the first of
the sha256/tarball lines immediately after the quotation breaks the flow.
>> +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 :)
Fixed!
>> + 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
I wasn't aware of this. Now fixed.
>> + curl -O "${TBB_DIST_URL}/${tarball}"
>
> Please use long option names to ease reviewing, auditing and
> future maintenance.
Fixed!
>> +# We'll use the en-US bundle as our basis ...
>
> Spurious space before "...".
Thanks!
>> +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.
I opted for explicitly extracting the expected folder. If there's a name
change, tar will now fail with an informative error message.
>> +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.
Thanks!
>> +# 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.
Well, I meant it as two separate reasons for doing that. I now realize
the "stand-alone" in the second part creates some overlap with the first
part, which may cause confusion. Would removing "stand-alone" make it
clearer?
>> +# 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.
Thanks!
>> +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 :)
I'm not sure simple grep:ing (or similar) is enough since the "<em:id>"
tag isn't unique (adblock-plus has several, for instance). Well, perhaps
the indentation level can be relied on for that, but I really don't know.
>> +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.
True. I switched to `cp -a` to keep inode info at least, but hidden
files are still not dealt with. Switching to `rsync -a` caused issues
(see below)...
>> +rm -rf "${TMP}"
>
> If `rm -r' works, better avoid the -f that will hide errors from `set -e'.
Right. Bad habits die hard.
> 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?
Sure. I couldn't come up with a place where we already do this. Do you
have any suggestion for a good location? /etc/live/config.d?
>> + 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).
They should be escaped, yes. Fixed!
>> - 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?
I thought this would be a good opportunity to lessen the configuration
delta with the TBB. In this instance it means that we don't have to look
out for upstream changes that depends the SOCKSPort value they use,
which IMHO is valuable.
>> +cp -r /etc/tor-browser/profile "${USER_PROFILE}"/profile.default
>
> I would use rsync and preserve more inode information.
I switched to `rsync -a`, but for whatever reason the destination didn't
get the "extensions" sub directory. Therefore I just switched to juse
`cp -a`. Any ideas?
>> +# Use the Tor Browser's hardcoded relative-patch only default profile
>
> s/patch/patch/ ?
I think you did the same mistake as me there. :) I've re-written the
comment completely (and it's moved together with the code into
/usr/bin/tor-launcher) since it wasn't very clear.
>> + mkdir -p /usr/Data/Browser/profile.default/preferences
>
> /usr/Data/, really?
Yes, really! :S
> Is that the "hardcoded relative-patch only default
> profile dir" mentioned above?
Exactly.
> Maybe a symlink would allow to put the
> actual files in a more relevant place, while keeping compatibility
> with the TBB's code?
Symlink how? Since -profile doesn't work together with -app,
/usr/Data/... will be used for the profile no matter what. Well, the
profile directory is relative to the application.ini file passed via
-app, so we can control where it ends up by putting the .ini file in an
appropriate place. One of the first things I tried was to create the
following symlink:
/home/tor-launcher/.tor-launcher/application/tor-launcher-standalone ->
/usr/share/tor-launcher-standalone
but the Tor Browser isn't fooled by symlinks and finds the real path.
This would work if we'd copy all of /usr/share/tor-launcher-standalone
into the place of the symlink instead... which isn't so bad since it's
only 325 KB in size (and so increased RAM usage because tmpfs). Do you
think commit f33296b improves the situation?
Or do you mean to simply make /usr/Data into a symlink pointing into
somewhere in /home/tor-launcher?
>> +Comment=Anonymous Web Browser
>
> Spurious trailing white-space.
Thanks!
>> +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?
Uh, that was me reading the man page (or whatever it was I read) wrong I
guess. I only pass --force-confold now.
> Great work!
Great review! Thanks again!
Cheers!
