Hi,
first, thanks a lot for this great work!
I've reviewed the intentions and the code. The former looks perfectly
fine to me, the latter is a very good start, but could use some
improvements here and there => see below.
I have *not* built an ISO from this branch, nor tested it yet.
I'll wait for the code changes I am suggesting to be settled
upon first.
Kill Your TV wrote (01 Aug 2014 00:30:19 GMT) :
> The consensus has been that that it would be good to require a user to
> explicitly acknowledge that they want access to I2P in order for it to
> be easily accessible within the system.
Right. Created #7722 to track the first step, aimed at 1.1.1.
> From what I understand, having it as an option in the greeter isn't
> possible until the new greeter is available.
Correct. Created #7723 to track this.
I'm wondering if using dpkg-divert (with the --rename option) would be
more robust than mv'ing stuff around. In particular, if at some point
we upload a newer I2P to a release's APT suite, and someone (either
manually, or via live-additional-software.conf) upgrades this package,
then they'll get the files that we moved around back in their original
place. Granted, this is an unlikely situation, and I may very well be
overdoing it, but I'm not sure if it's worth taking any chance
with it.
> +# Let's make sure that *just* the "i2psvc" user has access to the I2P files
> +chown -R i2psvc:i2psvc /usr/share/i2p
> +find /usr/share/i2p -type f \( -name '*.jar' -o -name '*.war' \) -print0 | xargs -r -0 chmod 640
It's unclear to me what doing this buys us.
Also, it's tricky to revert this kind of changes a robust way.
And indeed, the code that reverts it (in
/lib/live/config/2080-install-i2p) seems broken already (it does
"chown -R i2psvc:i2psvc /usr/share/i2p"), which, I guess, should make
my point clear enough.
*If* we really want to do something like this, I recommend looking at
dpkg-statoverride instead.
> +mv -f /usr/share/i2p "$DEST"
> +mv -f /usr/sbin/wrapper "$DEST/i2p"
> +mv -f /usr/share/applications/i2p.desktop "$DEST/i2p"
I'm a little bit concerned that we're mixing, in $DEST/i2p, any
content that came from /usr/share/i2p with content that comes from
other places, which feels fragile. E.g. if a "wrapper" file appears
someday in /usr/share/i2p, then this will break. I recommend instead
setting DEST=/usr/share/tails/i2p-disabled and moving everything
in there.
> + chmod 644 /etc/sudoers.d/zzz_i2p
The convention for files in that directory is 0440 instead.
> +Starting with Tails v1.1.1, I2P is not enabled by default when Tails starts.
"Tails 1.1.1" would be more consistent with how we do it
everywhere else.
Thanks for thinking of updating the design doc! I've some nitpicking
to do about it, though:
> +Starting with Tails v1.1.1, I2P is not enabled by default when Tails starts.
> +In order to use I2P, add the <span class="command">i2p</span> boot option
> +to the <span class="application">boot menu</span>. For detailed
> +instructions, see the documentation on [[using the <span
> +class="application">boot menu</span>|first_steps/startup_options#boot_menu]].
This phrasing looks adequate for end-user doc, but not for design
documentation. I suggest e.g. "a user must add [...]" instead.
Also, this section should point to the files that implement the new
disabling/re-enabling system, firewall rules handling, etc. I think
contribute/design/Tor_enforcement/Network_filter.mdwn needs an update
too. And design/I2P
Regarding the firewall rules:
Why does i2psvc need direct access to Tor's DNSPort?
> daddr 127.0.0.1 proto tcp syn mod multiport destination-ports (2827 4444 4445 6668 7656 7657 7658 7659 7660 8998) {
> - mod owner uid-owner amnesia ACCEPT;
> + @if $use_i2p mod owner uid-owner (amnesia i2psvc) ACCEPT;
> + }
I suspect that i2psvc doesn't need access to *all* of these ports.
That's certainly not a blocker for merging this branch, but maybe
it's worth creating a Research ticket about it?
> Other changes:
> - plugins will be disabled within I2P. Plugins, without persistence
> aren't very useful. Now that I know the switch to disable them,
> let's do it. (Users that want to use them can easily enable them)
> - documentation updates (more to come, but maybe not for these
> changes?)
> - the webmail app is changed to keep emails on the mail server.
> The default setting was changed upstream to store emails
> locally.(I'm submitting this one since it's low risk for Tails and
> it greatly improves user experience/expectations). This change
> depends on I2P 0.9.14 or newer. A point release (0.9.14.1) should
> be ready a few days before the Tails freeze.
I would usually ask separate branches for such unrelated changes, but
all of this makes sense to me, so let's cut on the paperwork for this
time :)
> If these changes are acceptable, please consider them for inclusion in
> the upcoming 1.1.1 release. Is there anything else that is desirable
> with respect to I2P & 1.1.1?
> (I presume that the separate browser will be too much for a point
> release,
Not necessarily, but let's polish and merge this initial branch first,
and then we'll see how much time we've left until the freeze :)
> but I'm going to start on that next for (potential) inclusion in
> 1.2.)
You rock! \o/
Cheers,
--
intrigeri
