Hi,
anonym wrote (20 Sep 2014 01:20:00 GMT) :
>>> +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.
Ah, right, got it. Actually, the installed (fake) package has priority
100, but it has a version higher than the one in Debian, and APT won't
downgrade "unless the priority of an available version exceeds 1000".
I was confused by the comment: it's not about blocking *installation*
of the iceweasel package (since we do want our fake one to be
installed), it's about blocking "upgrade" from our fake package to the
version we have in our repo. Given this, and what we actually want to
express is "don't upgrade the installed iceweasel package from an APT
repo, ever", I suspect a better, more generic "Pin" value could be
set: the current thing relies on the fact that the priority of all
repos that carry iceweasel is lower than 1000, except the ones we
explicitly tell about here. This seems to express in a quite
convoluted way what we want to say, and is also slightly fragile.
Something like:
Explanation: keep our fake equivs-generated iceweasel package
Package: iceweasel
Pin: origin ""
Pin-Priority: 1020
might be just enough to express exactly what we want to say
(untested), in a way that would work in more situations.
>> 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?
I would suggest *naming* operations that are being done, using
functions. Currently, one really has to read all comments to have
a broad idea of what the code is doing. While doing this, one would
also clarify what's the input and output of each operation, instead of
relying that much on incrementally built global state.
E.g. I would create a download_and_verify_files() function, taking
a base URL, a destination directory, and a list of (filename, expected
hash) -- or drop the base URL and pass the full URL for each filename.
I would create a create_and_install_fake_iceweasel_pkg() function,
that would take a version number as its single parameter, and would
use its own tmpdir instead of global state, etc. At least naming the
major steps of the process would help, even if we're still using a lot
of global state :)
But oh well, it feels strange to pretend I can teach you anything wrt.
software design and refactoring, you already know all this :)
>>> +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.
OK, makes a lot of sense. Make it clear in a comment? (I think we
should aim at the smallest possible delta with the TB here, so
documenting *why* this and that bit of our delta is needed will help
whenever we try to make the delta smaller in the future, and someone
will be asking exactly this kind of questions :)
> 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.
Fair enough.
[... snipping a lot of "Fixed!" replies from anonym -- 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?
Ah, I got it. How about: "We don't want tor-launcher to be part of the
regular browser profile. Moreover, for the stand-alone tor-launcher we
use, we need our patched version. So, the version shipped in the TB
really is not useful for us."
> 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.
OK, let's forget it.
>> 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?
I'm unsure whether we really want to export all these variables as
part of the global system-wide environment. I would instead store them
somewhere that can be sourced when needed.
We already do similar things in auto/config (saving stuff to
/etc/amnesia/) and auto/build (saving stuff to
/usr/share/amnesia/build/). I think that /usr makes more sense than
/etc, as what we want to save here is really static information about
how/where vendor-provided software is setup in the ISO, rather than
configuration => /usr/local/lib/tails-shell-library/tor-browser.sh,
maybe? It might be that we need more than variables in there at some
point, so bootstrapping a mini-shell-library with these doesn't seem
too crazy.
>>> - 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.
OK, I agree. The contribute/design/stream_isolation page needs an
update, then.
>>> +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?
I would need to know exactly what you did to be able to help.
>>> + 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.
OK, got it.
> 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?
At least it makes things a bit clearer. But then, why don't we:
* directly install the torbrowser-launcher code in the directory (in
tor-launcher's $HOME directory) where it will have to be run from
at ISO build time? once chroot_local-hooks/06-adduser_tor-launcher
has been run, that $HOME directory exists for us to put stuff in;
* do the profiles.ini etc. mangling at ISO build time too
?
> Or do you mean to simply make /usr/Data into a symlink pointing into
> somewhere in /home/tor-launcher?
Yep, that's what I meant initially, but I think we now have better
ideas :)
Here are more comments.
You'll want to:
* add /usr/local/bin/tor-browser to $SHELL_PROGS in the
refresh-translations script
* chmod +x config/chroot_local-includes/usr/local/bin/{tor-browser,generate-tor-browser-profile}
Regarding commit e554c1437:
* please use set -eu
* please use the shell pipefail option
* please check that the output is non-empty before returning it,
and exit 1 otherwise
... else, we're losing quite some of the robustness of the previous
Perl script.
Cheers,
--
intrigeri
