[Tails-dev] Reviewing kytv:feature/i2p-0.9.8.1 [Was: about t…

Delete this message

Reply to this message
Autore: intrigeri
Data:  
To: The Tails public development discussion list
Vecchi argomenti: Re: [Tails-dev] about the maintenance of I2P in Tails
Oggetto: [Tails-dev] Reviewing kytv:feature/i2p-0.9.8.1 [Was: about the maintenance of I2P in Tails]
Hi,

Kill Your TV wrote (08 Nov 2013 14:52:33 GMT) :
> On Mon, 4 Nov 2013 11:41:50 +0000 (UTC)
> intrigeri <intrigeri@???> wrote:


> [...]


>> I think the best way to handle it is:
>>
>>   * killyourtv maintains a Tails-experimental suite in their APT
>>     repository.


> Done. The sources line is:


>        deb http://deb.i2p2.no tails-experimental main


> As promised previously, the packages in this repo will not change until
> after the next Tails release.


Once we have pulled the relevant packages from there into our own
repo, you are welcome to unfreeze this APT suite :)

>>   * Optional: we patch auto/scripts/tails-custom-apt-sources to
>>     include this suite when building from our experimental branch.
>>     This makes the next step easier.


> I haven't done this (but could take a stab at it).


Cool. The relevant Cucumber feature (features/build.feature) should help.

>>   * When s/he wants Tails to get a newer version of I2P, killyourtv
>>     uploads it to their Tails-experimental APT suite and asks for
>>     a review'n'merge. Most of the times, no change is needed in Tails
>>     source code, but when it's needed, then the review'n'merge request
>>     must include a feature/i2p-$version branch.


> I think I'm ready to have other eyes on it. I branched off of 'devel' a
> few days ago.


>      http://repo.or.cz/w/tails/kytv.git/shortlog/refs/heads/feature/i2p-0.9.8.1


> My commit comments tend to be verbose but hopefully they're not _too_
> wordy for your use.


IMHO it's hard to have too verbose commit messages.

Also, next time, please set the email subject so that it's clear it is
a review'n'merge request (e.g. in case someone has no time to read the
entire thread, but still wants to keep an eye on what is proposed for
merging once conclusions have been reached).

Here is a first review review pass.

First, congrats, impressive work!

1. Regarding adding I2P IRC channels: great; please not that, without
a proper upstream solution in Pidgin that allows the user to
understand what the non-obvious @127.0.0.1 and @XXX.onion accounts
are (Tails#6232), I'm not sure we will keep these enabled for long.

2. Why the (undocumented) switch from `set -e' to `/bin/sh -e'?
I generally prefer the former, as it's in effect even when someone
mistakenly runs the script e.g. with "sh $SCRIPT".

3. I read this:
> * Disable "in-network" updates (this is also done in the regular I2P packages)
What are "in-network" updates?
What are the "regular I2P packages"?

4. I read this:
> * relaxed permissions so that both the i2psvc user and the i2psvc group have access
Access to what, and why is it useful / needed for?
I assume this documents this change:
> # Loosen wrapper permissions so that the amnesia user (who'll be added to the i2psvc group) has access
> sed -i 's|^.*\(wrapper.*umask=\).*|\10007|g' "$WRAPPER"
and:
> # * insecure files: Enabled. This means that permissions will not be forced
> # to 700 for the i2psvc user, giving i2psvc group members access.
... that don't tell me much either.

A further commit reads "Add the amnesia user to the i2psvc group",
"This will allow the standard Tails user to access the I2P config
directory." Same question: what is it useful for? Does this *only*
adds access to that directory, or does it gives the desktop user
other credentials as a side effect? If access to that directory is
really needed, and only that, perhaps we could use an ACL instead?

5. I read this:
> * Boostrap through 127.0.0.1:8118

This is an important change to how Tails has been using I2P until
now. If our brand new I2P maintainer says it's better to have it go
through Tor, I'm very happy. Is it now *entirely* going through
Tor, that is, can we drop the firewall exception that allows I2P to
go out in the clear, and update the design doc accordingly? Or is
it only the bootstrap step that goes over Tor?

Also, is it possible to use the Tor SOCKS proxy, rather than going
through polipo? (We're at the point we can almost ditch it
entirely, and stop shipping a HTTP proxy in Tails, so adding
a usecase for it makes me sad.)

6. This is pretty bold affirmation:
> # Settings that are set or unset in this file will be useful for all versions
> # of Tails.
... but well :)

7. I see /usr/share/i2p/docs/initialNews/initialNews.xml uses some
gettext'ish _("") syntax, but I don't see how and when the strings
are extracted. See the refresh-translations script.

8. I read "Add exceptions to ferm for the standard I2P ports".
Please document what these ports are useful for.

9. In the revised design doc, I read:
> The I2P hosts an [APT repository](http://deb.i2p2.no)
Missing word, or is it me lacking knowledge about I2P?

10. I also read:
    > 2. The local *eepsite*: urls matching one of the
    >   `http://127.0.0.1:7658/*` and `http://localhost:7658/*` wildcard
    >   patterns will get a direct connection to the local host so the locally
    Does the 2nd form really work? I see no such thing in
    FoxyProxy config?


11. The design doc update drops the fact we still plan to import your
    Debian packages into our own APT repo. I think it's losing
    valuable information.


> The repository's key for secure apt is also attached and signed by me.
> The repository key is also packaged at
> http://deb.i2p2.no/pool/main/i/i2p-keyring/.


Thanks!

Cheers,
--
intrigeri
| GnuPG key @ https://gaffer.ptitcanardnoir.org/intrigeri/intrigeri.asc
| OTR fingerprint @ https://gaffer.ptitcanardnoir.org/intrigeri/otr.asc