Re: [Tails-dev] Reviewing kytv:feature/i2p-0.9.8.1 [Was: abo…

Delete this message

Reply to this message
Author: Kill Your TV
Date:  
To: tails-dev
Subject: Re: [Tails-dev] Reviewing kytv:feature/i2p-0.9.8.1 [Was: about the maintenance of I2P in Tails]
On Sat, 9 Nov 2013 13:28:49 +0000 (UTC)
intrigeri <intrigeri@???> wrote:


> > 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 :)


I figured.

> >>   * 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.


Excellent.

>
> > 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.


I agree but some have complained in the past so until I hear otherwise
I default to verbose.


> 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).


Understood.

> Here is a first review review pass.
>
> First, congrats, impressive work!



TY :) (There are more things that need addressing than I would like;
some of which I noticed _after_ I sent the email (as luck would have
it)).

> 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.


It's not a problem if it goes away.

> 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".


Good point. I'll explicitly set it on its own line. (Why was it done? A
bad habit that I need to break from.)


> 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"?


In-network updates are how non-packaged versions of I2P receive
updates. Since the packaged versions have a read-only installation
directory the "in-network" updates would be disabled anyway.

By "regular packages" I meant, in essence, "any time that I2P is
installed from a .deb. That is so say, this is not a change from
upstream."

> 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.


It was intended to give access to /var/lib/i2p to the standard amnesia
user so that s/he could make changes without needing 'sudo' By default
the Java Service Wrapper is configured thusly:

# Set permissions used when creating files
# See http://wrapper.tanukisoftware.com/doc/english/prop-umask.html
# for a detailed explanation of these settings.
wrapper.umask=0022
wrapper.java.umask=0022
wrapper.logfile.umask=0077

This effectively means that the i2psvc user is the only one with
read/write access. Others, including the i2psvc group, cannot write
to /var/lib/i2p. Potential problems with this default set-up for a
Tails user who does not set a password prior to logging in:

- If a Tails user tried to start I2P and it failed for some
reason, s/he may be advised to look in /var/log/i2p. Due to the default
umask set, the amnesia user cannot access it.
- If a Tails user (for whatever reason) decided to download an
I2P-internal torrent, any such files would be lost.
- If a Tails user wanted to set up an 'eepsite' (be it temporarily to
share something quickly or something a bit more long term once we
have persistence enabled), s/he wouldn't be able to access that
folder


>    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?


The only extra access /should/ be only to access that directory without
requiring admin access being set upon logging in. If we have ACLs
available to us with Tails, this indeed would be another way to solve
this.


> 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.)


Only the initial reseeding/bootstrapping would happen over Tor.
Using a SOCKS proxy isn't possible yet but I can file a wishlist bug
for it.

> 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 :)


Oops, that should have been removed when I combined several of my
scripts into 16-i2p_config. Bold indeed. It wouldn't have seemed so
bold in the original (unchecked-in) version.


> 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.


OK.

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


OK.

> 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?



It's either a missing word or one too many. I'll correct this at the
same time as #11. (*Sigh*)

> 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?


It worked in testing but I'll make it explicit.


> 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.


Yes, I'll add it.