[bug#42380] [PATCH v5 9/9] gnu: Add torbrowser-unbundle.

["André Batista" <nandre@riseup.net>]

[-- Attachment #1: Type: text/plain, Size: 9776 bytes --]

Hi Maxim,

I don't know why but I've overlooked your message when it first arrived,
I'm sorry and thanks for your help.

qui 03 jun 2021 às 23:07:23 (1622772443), maximedevos@telenet.be enviou:
> > + GenericName=Web Browser
> > + Comment=Tor Browser is +1 for privacy and −1 for mass surveillance
> > + Categories=Network;WebBrowser;Security;
> > +-Exec=sh -c '"$(dirname "$*")"/Browser/start-tor-browser --detach || ([ ! -x "$(dirname "$*")"/Browser/start-tor-browser ] && "$(dirname "$*")"/start-tor-browser --detach)' dummy %k
> > +-X-TorBrowser-ExecShell=./Browser/start-tor-browser --detach
> > +-Icon=web-browser
> > ++Exec=sh -c start-tor-browser
> > ++X-TorBrowser-ExecShell=start-tor-browser --detach
> > ++Icon=torbrowser
> 
> What's the reason for switching the icon from web-browser to torbrowser?

If I'm not mistaken, this was done to get the newer icon set from sources.
Without this change it was getting the older icons which were still
available. I'll check this again.

> Also, the guixy way would be to simply replace "$(dirname "$*")/STUFF"
> by /gnu/store/[...]/MORE-STUF/STUFF.
> Otherwise, you're assuming "sh" is in the profile. It would also
> be possible to replace "sh" with (string-append (assoc-ref inputs "bash") "/bin/sh")
> I guess.

You are right, I'm getting warnings on sh or bash when compiling.

> > + # First, make sure DISPLAY is set.  If it isn't, we're hosed; scream
> > +@@ -134,8 +142,8 @@
> > +           ;;
> > +       -l | --log)
> > +           if [ -z "$2" -o "${2:0:1}" == "-" ]; then
> > +-             printf "Logging Tor Browser debug information to tor-browser.log\n"
> > +-             logfile="../tor-browser.log"
> > ++             printf "Logging Tor Browser debug information to torbrowser.log\n"
> 
> Why rename tor-browser.log to torbrowser.log?

I think this had no reason other than my personal choice.

> > + [...]
> > ++# Try to be agnostic to where we're being started from, check if files are on its
> > ++# default paths and chdir to TBB_HOME
> > ++if [ -e "${TORRC}" ]; then
> > ++   cd "${TBB_HOME}"
> > ++else
> > ++   mkdir -p "${TBB_HOME}"
> > ++   cp -R "${TBB_STORE_DATA}" "${TBB_HOME}"
> > ++   chmod -R 700 "${TBB_HOME}"
> > ++   mkdir -p "${TBB_PROFILE}"
> > ++   echo "user_pref(\"extensions.torlauncher.torrc-defaults_path\", \"${TORRC}\");"\
> > ++     > "${TBB_PROFILE}/user.js"
> > ++   echo "ClientTransportPlugin meek_lite,obfs2,obfs3,obfs4,scramblesuit exec ${TBB_STORE_PATH}/TorBrowser/Tor/PluggableTransports/obfs4proxy"\
> > ++     >> "${TORRC}"
> > ++   cd "${TBB_HOME}"
> > + fi
> 
> "mkdir" and "cp" are from coreutils, which are not guaranteed to be in
> the profile. I'd suggest:
> 
> (1) (preferred) use substitute* in a build phase to replace
>     'mkdir' and 'cp' & co with the absolute store path
> (2) or add coreutils to propagated-inputs
> 
> Likewise for sed. 

Noted. I'll write a build phase.

> > [...]
> > 
> > + if [ "$register_desktop_app" -eq 1 ]; then
> > + 	mkdir -p "$HOME/.local/share/applications/"
> > +-	cp ../start-tor-browser.desktop "$HOME/.local/share/applications/"
> > ++	cp "${TBB_STORE_PATH}/start-tor-browser.desktop" "$HOME/.local/share/applications/"
> > + 	update-desktop-database "$HOME/.local/share/applications/"
> > + 	printf "Tor Browser has been registered as a desktop app for this user in ~/.local/share/applications/\n"
> > + 	exit 0
> 
> Is this required on Guix and would it work well on Guix? Copying .desktop files around
> seems counter to ‘Guix suppots transactional upgrades and roll-backs, [...]. [...]
> reproducible operating systems’ and not very functional. Shouldn't
> "guix install torbrowser-unbundle" be sufficient?

Yes, it should. I think I was trying to change things as little as
possible from what the official bundle does/provides, but I agree that
it would be better to just remove this part of the script and have it
behave as an ordinary application.

> noscript seems an useful extension for IceCat as well.
> Maybe move it to gnuzilla.scm? Maybe move https-everywhere
> there as well? (Separate issue: https-everywhere seems to
> be bundled in IceCat ...)

I was thinking the better outcome would be to have something as it
was done for ungoogled-chromium to provide extensions that would
be compatible with both icecat and torbrowser. Unfortunatly, I've
been unable to address most of the things I had intended for now
at least. I'll also keep a note on this and address it as soon as
I can.

> Why are you copying the source code to somewhere else?

Because I did not know any better.

> This doesn't seem to accomplish anything. I would suggest
> something like:
> 
> ;; Source code of ‘HTTPS Everywhere WASM Library’,
> ;; licensed as license:lgpl2.1+, used in 'https-everywhere'
> ;; as an input.
> (define https-everywhere-lib-wasm/source-code
>   (origin (method git-fetch) [...]))
> 
> Note that you can use 'origin' objects in 'inputs' and 'native-inputs'.

Thanks, I did not know this. I'll change accordingly.

> > +      (license license:gpl2+))))
> 
> The license file seems to tell something different: LGPL2.1+ instead of GPL2+:
> https://github.com/EFForg/https-everywhere-lib-wasm/blob/master/LICENSE

Ouch. You are right.

> > [...]
> > +      (native-inputs
> > +       `(("https-everywhere" ,https-everywhere)
> > +	   ("noscript" ,noscript)
> 
> noscript and https-everywhere seem more like 'inputs' than
> 'native-inputs' to me, but IIUC they are source-code only
> and not compiled, so it doesn't really matter here I guess.

Noted.

> > +                   (make-file-writable (string-append tbb-scripts-dir
> > +						      "/RelativeLink/start-tor-browser.desktop")))
> > +		 #t))
> 
> Returning #t at the end of a phase is not required anymore.
> The warning will disappear when core-updates is merged.

Noted.

> > +             (replace 'configure
> > +               (lambda* (#:key inputs outputs configure-flags #:allow-other-keys)
> > +		 (let* ((out (assoc-ref outputs "out"))
> > +			(bash (which "bash"))
> > +			(flags `(,(string-append "--prefix=" out)
> > +				 ,@configure-flags)))
> > +                   (setenv "SHELL" bash)
> > +                   (setenv "AUTOCONF" (string-append
> > +                                       (assoc-ref %build-inputs "autoconf")
> > +                                       "/bin/autoconf"))
> 
> In build phases, use 'inputs' or 'native-inputs' instead of %build-inputs.
> It's more explicit, maybe there are other reasons as well. (Here it should
> be native-inputs I guess).

Noted.

> > +                   (setenv "CONFIG_SHELL" bash)
> > +                   (setenv "PYTHON" (string-append (assoc-ref inputs "python-2")
> > +                                                   "/bin/python"))
> 
> This most likely should be (assoc-ref (or native-inputs inputs) "python-2")
> instead of (assoc-ref inputs "python-2").
> 
> > +                   (setenv "CC" "gcc")  ; needed when Stylo is enabled
> This most likely should be ,(cc-for-target) instead of "gcc".
> 
> (The native-inputs/inputs and "gcc" / (cc-for-target) distinction is important
> when cross-compiling (though not all dependencies are cross-compilable currently,
> so it's a bit moot for now.))

Noted. This and the previous code snippets came from Thunderbird if I
remember correctly and I didn't really give it much tought. I'll try to
address them both.

> > +             (add-after 'install-extensions 'link-binaries
> > +               (lambda* (#:key inputs native-inputs outputs #:allow-other-keys)
> You're not using 'native-inputs' in this build phase so you can remove it
> from the arguments list.
> > +              [...]
> > +             (add-after 'link-binaries 'copy-bundle-data
> > +               (lambda* (#:key inputs native-inputs outputs #:allow-other-keys)
> 
> Likewise.

Noted.

> > +      (description
> > +       "Tor Browser is the Tor Project version of Firefox browser.  It is the only
> > +recommended way to anonymously browse the web that is supported by the project.
> > +It modifies Firefox in order to avoid many know application level attacks on
> > +the privacy of Tor users.
> > +
> > +WARNING: This is not the official Tor Browser and is currently on testing.  Use
> > +at your own risk and please report back on guix channels if you find any
> > +issues.")
> 
> This seems unnecessarily scary. All packages in guix are ‘at your own risk’
> and every new package is ‘in testing’ for a while, whatever that means.
> What about

I wrote this to give some consideration to the fact that TorProject
strongly advises people _NOT_ to build their own TorBrowsers and not
to use anything other than the one they provide. I've also intended to
make it clear that the anonymity set might be reduced and that guix
users could be distinguished from other TB users, specially given the
nature of guix would imply that at sometime or other guix users would
even be using different software versions, which applies to extensions,
libs and even the tor software under the hood. I also gathered that
it would be sometime before this could be merged into guix and that
maybe some users would find this recipe on the mailing lists and
install themselves, so I thought it would be a good thing to be over
scary rather than give a false impression that it was something one
should do without a second thought.

Thanks for your valuable input. I've been a little short on time and
got stuck for a while, but I do intent to go back to it and when I
do I'll contemplate your suggestions.

Cheers,


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 667 bytes --]