Project

General

Profile

Feature #16664

Simplify "Tor has bootstrapped" status check

Added by segfault 7 months ago. Updated about 1 month ago.

Status:
Resolved
Priority:
Normal
Assignee:
Category:
-
Target version:
Start date:
Due date:
% Done:

100%

Feature Branch:
feature/16664-simplify-tor-has-bootstrapped+force-all-tests
Type of work:
Code
Blueprint:
Starter:
Affected tool:

Description

intrigeri and I started discussing this in #16169#note-14 and following.


Related issues

Blocks Tails - Feature #16903: Refactor tails-documentation In Progress
Blocked by Tails - Feature #16935: Move tailslib to main repo Resolved

Associated revisions

Revision d9ae8bf8 (diff)
Added by segfault 4 months ago

Use BindsTo= and After= in tor-has-bootstrapped systemd units (refs: #16664)

Currently, if stops for some reason (either stopped
manually or unexpectedly), tails-tor-has-boostrapped.target is still
active.

Using BindsTo= in conjunction with After= ensures that the unit is
always stopped if the other unit () is stopped.
See https://www.freedesktop.org/software/systemd/man/systemd.unit.html#BindsTo=

This allows us to simplify config/chroot_local-includes/usr/local/sbin/tor-has-bootstrapped,
which would only have to check if tails-tor-has-bootstrapped.target is active.
Or, we could get rid of this script altogether, because instead of
calling the script, applications can just run

/bin/systemctl --quiet is-active tails-tor-has-bootstrapped.target

themselves.

Revision 7f7757cd (diff)
Added by segfault 4 months ago

Remove tor-has-bootstrapped script (refs: #16664)

Replace all calls to config/chroot_local-includes/usr/local/sbin/tor-has-bootstrapped
with `/bin/systemctl --quiet is-active tails-tor-has-bootstrapped.target`.

Revision 3cdb6d5a (diff)
Added by segfault 4 months ago

Remove obsolete manual stops of tails-tor-has-bootstrapped.target (refs: #16664)

is stopped above the removed lines, which now
automatically stops tails-tor-has-bootstrapped.target.

Revision 34e4e3f2
Added by intrigeri about 1 month ago

Merge branch 'feature/16664-simplify-tor-has-bootstrapped+force-all-tests' into devel (Closes: #16664)

History

#1 Updated by segfault 7 months ago

  • Description updated (diff)

In reply to #16169#note-18:
intrigeri wrote:

intrigeri wrote:

Actually, I don't understand why it has to check the status of tor\@default.service. IIRC we correctly stop tails-tor-has-bootstrapped.target whenever we stop tor. Perhaps this wrapper is erring on the safe side, which makes some sense.

No, if I run systemctl stop tor\@default.service after it has bootstrapped, the current tails-tor-has-bootstrapped.target is still active (because it doesn't use BindsTo). I think that's why we currently also check the status of tor\@default.service.

Sorry I was unclear! My claim/guess/assumption was about what happens automatically, i.e.:

  • whenever Tails stops tor\@default.service, it also stops tails-tor-has-bootstrapped.target
  • we restart tails-tor-has-bootstrapped.target conservatively every time the network goes up or back up

IMHO we should not worry about the case when a user manually stops tor\@default.service here, and thus checking tails-tor-has-bootstrapped.target should be enough. Anyway, I'm kinda diverging off topic here :)

BindsTo doesn't only take care about the case that tor\@default.service is stopped manually, but also that it stops unexpectedly.

On the one hand, one less unit decreases complexity. OTOH, a target — a "well-known synchronization point" — feels like the exact semantics we want here.
Note that the reason we introduced a target in c195ff18add2ef18be45e133678b5895d8dce255 was #9393.

The way I understand it, a systemd service can be used in exactly the same way as a systemd target. Both are units, and a target is just a unit without any special properties, see https://www.freedesktop.org/software/systemd/man/systemd.target.html. So we should still be able to use it as a well-known synchronization point.

Sure, technically a service will work just the same, at least in the current state of systemd. It's just that it's not meant to be used this way, as per the doc:

  • A unit configuration file whose name ends in ".service" encodes information about a process controlled and supervised by systemd
  • A unit configuration file whose name ends in ".target" encodes information about a target unit of systemd, which is used for grouping units and as well-known synchronization points during start-up.

So using a service to provide a well-known synchronization point can be somewhat confusing. But that's certainly not a blocker IMO!

OK, so it does boil down to the trade-of "one less unit to decrease complexity" vs "the unit type conveys that this is a well-known synchronization point". I still prefer the "one less unit", but I would also be OK with having an additional tails-tor-has-bootstrapped.target like this (assuming that we then name the service tails-wait-until-tor-has-bootstrapped.service):

[Unit]
Description=Tor has Bootstrapped
Documentation=https://tails.boum.org/contribute/design/
After=tails-wait-until-tor-has-bootstrapped.service
BindsTo=tails-wait-until-tor-has-bootstrapped.service

[Install]
WantedBy=tails-wait-until-tor-has-bootstrapped.service

I didn't test this yet, but I assume that this is automatically started/stopped whenever the tails-wait-until-tor-has-bootstrapped.service turns active/inactive.

I don't think we would have to change any existing AppArmor profiles. None of the other scripts which use /usr/local/sbin/tor-has-bootstrapped seem to be confined by AppArmor. I also tested that tails-documentation is also allowed to execute systemctl is-active (even though I couldn't find an AppArmor for it, so I guess it's not confined at all).

How does that work for opening links from Evince, Pidgin, and Totem? I don't know if they're all allowed to execute systemctl (with any argument).

  • Evince can't start Tor Browser at all currently, because it's not allowed to execute /usr/local/bin/tor-browser (I tested this by downloading and opening http://gahp.net/wp-content/uploads/2017/09/sample.pdf and clicking on the reference example, which shows "Unable to open external link" in evince).
  • Pidgin is allowed to execute systemctl
  • I couldn't find a way to open a link via Totem

right now… and every time we change the implementation details again in the future. (Yeah, I'd love this one to be the list time but assuming it'll be feels a tad presumptuous given the tortuous history of this topic in Tails.)

OK, I see that it could be a valid concern that the above systemd service might not be sufficient to tell if tor has bootstrapped if tor breaks it's behavior on that part. But I think the chances that even in that case there are good chances that we will be able to adapt the service so still work as expected.

OK, you've convinced me that from a design perspective at least, the status of that service can plausibly serve as a LTS API entry point, whose implementation details we can change without affecting its consumers. At least I'm OK to try it :)

Cool :)

#2 Updated by segfault 4 months ago

  • Status changed from Confirmed to In Progress

#3 Updated by segfault 4 months ago

#4 Updated by segfault 4 months ago

  • Status changed from In Progress to Needs Validation
  • Assignee deleted (segfault)

Tests pass on Jenkins

#5 Updated by intrigeri 4 months ago

  • Feature Branch set to feature/16664-simplify-tor-has-bootstrapped+run-all-tests

#6 Updated by intrigeri 4 months ago

  • Target version set to Tails_3.17

(Too invasive for a bugfix release IMO.)

#7 Updated by intrigeri 3 months ago

  • Target version changed from Tails_3.17 to Tails_4.0

If 3.17 is released, it'll be a bugfix release.

#8 Updated by intrigeri 3 months ago

  • Assignee set to intrigeri

I've merged current devel into the branch and solved the submodule conflict, so the branch builds again on Jenkins. All this, in the hope I manage to review this branch in time for 4.0~beta2 (although it's already too late, if I ask segfault to do some changes; sorry about that).

#9 Updated by intrigeri 2 months ago

intrigeri wrote:

I've merged current devel into the branch and solved the submodule conflict, so the branch builds again on Jenkins. All this, in the hope I manage to review this branch in time for 4.0~beta2

Ditto, modulo s/beta2/rc1/.

#10 Updated by intrigeri 2 months ago

  • Feature Branch changed from feature/16664-simplify-tor-has-bootstrapped+run-all-tests to feature/16664-simplify-tor-has-bootstrapped+force-all-tests

(Renamed the branch so Jenkins actually runs all tests.)

#11 Updated by intrigeri 2 months ago

#12 Updated by intrigeri 2 months ago

  • Status changed from Needs Validation to In Progress
  • Assignee changed from intrigeri to segfault

Blocked by Feature #16935: Move tailslib to main repo added

I'd rather merge #16935 first, then you can transplant your tailslib changes done here to tails.git. OK?
Feel free to adjust the target version as you see fit.

#13 Updated by segfault 2 months ago

I merged devel and tested that everything still works as expected. One test failed in Jenkins, which I expect is unrelated ("Scenario Booting Tails from a USB drive upgraded from DVD with persistence enabled"), but I'll wait for the next Jenkins test results before setting the status to "Needs Validation".

#14 Updated by segfault about 2 months ago

  • Status changed from In Progress to Needs Validation
  • Assignee deleted (segfault)

segfault wrote:

I merged devel and tested that everything still works as expected. One test failed in Jenkins, which I expect is unrelated ("Scenario Booting Tails from a USB drive upgraded from DVD with persistence enabled"), but I'll wait for the next Jenkins test results before setting the status to "Needs Validation".

In the meantime, there was one test run without failures (https://jenkins.tails.boum.org/job/test_Tails_ISO_feature-16664-simplify-tor-has-bootstrapped-force-all-tests/8/) and a few ones with a few failures. I assume that those tests were fragile before and the failures are unrelated to this change. @intrigeri, please tell me if you think that's not the case.

#15 Updated by intrigeri about 1 month ago

  • Assignee set to intrigeri

I'd really like to keep our amount of WIP under control, so even though segfault and I agree this is not particularly urgent, I'll do this review now, in the hope this can make it into 4.0~rc1.

#16 Updated by intrigeri about 1 month ago

I've re-read our past discussion, thought about the systemd implementation details a bit more, and tested various corner cases manually (e.g. offline mode, 1st tor bootstrap succeeds but then after switching networks tor fails to bootstrap). Conclusion: I like the current proposed implementation and I'm reasonably convinced it should not break stuff (famous last words :)

I've merged current devel into this branch so it builds again; I'm running the full test suite on it locally. I'll merge this branch if the test suite is happy!

One note in passing regarding a potential future problem, just so that I'm not the only one who can take this info into account. IIRC, systemctl talks with systemd over DBus. Currently, AppArmor mediation of DBus stuff is not upstream so we don't have it. Once AppArmor DBus mediation lands in Linux mainline (eventually…), we'll have to choose between:

  • Give various apps permission to do whatever DBus operations that systemctl is-active needs.
  • Re-introduce a wrapper script, that those apps are allowed to start unmediated; that is, reintroduce essentially /usr/local/sbin/tor-has-bootstrapped, that this branch gets rid of for the sake of simplicity.

But that's not a blocker at all, and as your tests showed wrt. Evince, Pidgin, and Totem, in the end "various apps" might be just 1 or 2, so yeah, whatever.
And well, worst case, we'll reintroduce the wrapper, and so be it. It's not a sufficient reason to keep the complexity now, just in case we might need it again later!

#17 Updated by intrigeri about 1 month ago

  • Status changed from Needs Validation to Resolved
  • % Done changed from 0 to 100

Also available in: Atom PDF