Bug #15472

Rebase our Tor Browser AppArmor policy on top of torbrowser-launcher 0.2.9-2's

Added by bertagaz 2018-03-28 20:19:07 . Updated 2018-09-05 16:08:59 .

Status:
Resolved
Priority:
High
Assignee:
Category:
Target version:
Tails_3.9
Start date:
2018-03-28
Due date:
% Done:

100%

Feature Branch:
feature/15472-torbrowser-apparmor-updates, torbrowser-launcher:feature/15472-torbrowser-apparmor-updates
Type of work:
Code
Blueprint:

Starter:
Affected tool:
Deliverable for:

Description

Apparmor patches fail to apply.

Subtasks

Related issues

Related to Tails - Feature #10422: Grant Tor Browser access to files as designated by the user Confirmed 2018-08-30
Blocks Tails - Feature #15139: Core work 2018Q2: Foundations Team Resolved 2018-01-01
Blocked by Tails - Bug #15622: Upgrade to Linux 4.16+ in Tails 3.9 Resolved 2018-05-28
Blocks Tails - Bug #15638: Remove deb.torproject.org sid APT source Resolved 2018-06-07
Blocks Tails - Bug #15116: X.Org does not start with some NVidia Maxwell and Pascal graphic cards Confirmed 2017-12-27
Blocks Tails - Bug #15627: feature/14594-asp-gui FTBFS due to merge conflicts Resolved 2018-05-29
Blocks Tails - Feature #8514: Replace WhisperBack.mail_appended_info with a dictionary Resolved 2015-01-02
Blocks Tails - Bug #15690: Stop installing all "Priority: standard" packages only to remove some of them later Resolved 2018-06-29

History

#1 Updated by bertagaz 2018-03-28 20:23:43

  • Target version changed from Tails_3.7 to Tails_3.9

Actually devel is supposed to target 3.9.

#2 Updated by bertagaz 2018-03-29 01:28:34

  • Status changed from Confirmed to In Progress

Applied in changeset commit:30b8aeb84fbfdef5aca46f28f6b379097bd5f689.

#3 Updated by intrigeri 2018-03-29 07:31:12

  • Assignee set to intrigeri
  • Target version changed from Tails_3.9 to Tails_3.7

I’ll take care of this. I took my sweet time so far because the branch FTBFS for another reason anyway, but as soon as my fix for that other failure is fixed I’ll handle this one.

Setting target version to something closer so that’s on my radar.

#4 Updated by intrigeri 2018-03-29 07:31:29

#5 Updated by intrigeri 2018-03-29 07:31:56

  • Priority changed from Normal to Elevated

#6 Updated by intrigeri 2018-03-29 07:43:30

  • Subject changed from devel branch FTBFS since torbrowser-launcher 0.2.9-2 entered sid to Rebase our Tor Browser AppArmor policy on top of torbrowser-launcher 0.2.9-2's

This problem will come back as soon as 0.2.9-2 enters stretch-backports, which I bet will happen in a few days.

#7 Updated by intrigeri 2018-03-29 10:11:08

  • % Done changed from 0 to 10
  • Feature Branch set to feature/15472-torbrowser-apparmor-updates

#8 Updated by intrigeri 2018-03-29 10:15:36

  • Feature Branch changed from feature/15472-torbrowser-apparmor-updates to feature/15472-torbrowser-apparmor-updates, torbrowser-launcher:feature/15472-torbrowser-apparmor-updates

I had to base my tbl topic branch on top of a gbp-generated patch-queue/debian/sid since the changes were imported in Debian as a quilt patch series. That’s painful so hopefully 0.3 (that’ll include these changes upstream) will be out and packaged soon => my branch is based on something more simple and intuitive.

#9 Updated by intrigeri 2018-03-29 11:14:54

Oops, I had blindly assumed that the short-term fix pushed by bertagaz straight to devel (commit:30b8aeb84fbfdef5aca46f28f6b379097bd5f689) had been successfully tested so I had moved on to the longer-term fix. But it does not work so I’ll fix it directly in devel as well.

#10 Updated by intrigeri 2018-03-29 12:59:56

So bertagaz and I fixed the FTBFS at about the same time. A bit more communication would be appreciated to avoid duplicating work. Anyway, moving on and now focussing on what I’ve repurposed this ticket for this morning.

#11 Updated by intrigeri 2018-03-30 07:37:22

  • Assignee changed from intrigeri to anonym
  • % Done changed from 10 to 50
  • QA Check set to Ready for QA

Please merge into devel. I’m leaving target version at 3.7 so that’s on top of your radar: if this does not get merged before torbrowser-launcher 0.2.9-2~+ reaches stretch-backports, our devel branch will start FTBFS’ing again, which would be unfortunate given a number of us are working hard on their SponsorW devel-based branches these days.

I’ve seen all automated tests pass at least once.

I’m not super happy with passing a non-path to the Onion Grater’s exe-paths. Shall we rename that variable?

I’m not 100% sure about commit:6b13c98159915cf89215a22e4eb795d787b9f349 because I could not (easily) find an explanation for the / I’m removing so I wonder if I’m breaking stuff.

#12 Updated by anonym 2018-04-26 11:40:39

intrigeri wrote:
> I’m not super happy with passing a non-path to the Onion Grater’s exe-paths.

Agreed!

> Shall we rename that variable?

Yes! I wonder how, though. The code needs a similar change since returning the AppArmor profile name doesn’t make sense in a function called exe_path_of_pid().

> I’m not 100% sure about commit:6b13c98159915cf89215a22e4eb795d787b9f349 because I could not (easily) find an explanation for the / I’m removing so I wonder if I’m breaking stuff.

I am sure I just overlooked that profile names could be there, assuming we’d always get absolute paths.

#13 Updated by intrigeri 2018-04-30 08:35:02

anonym wrote:
> intrigeri wrote:
>> I’m not super happy with passing a non-path to the Onion Grater’s exe-paths.

> Agreed!

>> Shall we rename that variable?

> Yes! I wonder how, though. The code needs a similar change since returning the AppArmor profile name doesn’t make sense in a function called exe_path_of_pid().

I propose:

  • s/exe-paths/apparmor-profiles/
  • s/exe_path_of_pid/apparmor_profile_of_pid/
  • some more strings and the doc need adjustments

Shall I go ahead?

Does anything else needs to be done here before we call this ready to be merged as soon as torbrowser-launcher 0.2.9-2~+ reaches stretch-backports?

#14 Updated by bertagaz 2018-05-10 11:09:33

  • Target version changed from Tails_3.7 to Tails_3.8

#15 Updated by intrigeri 2018-05-24 13:02:46

  • Assignee changed from anonym to intrigeri
  • QA Check deleted (Ready for QA)

intrigeri wrote:
> anonym wrote:
> > intrigeri wrote:
> >> I’m not super happy with passing a non-path to the Onion Grater’s exe-paths.
>
> > Agreed!
>
> >> Shall we rename that variable?
>
> > Yes! I wonder how, though. The code needs a similar change since returning the AppArmor profile name doesn’t make sense in a function called exe_path_of_pid().
>
> I propose:
>
> * s/exe-paths/apparmor-profiles/
> * s/exe_path_of_pid/apparmor_profile_of_pid/
> * some more strings and the doc need adjustments
>
> Shall I go ahead?

I’ll do this.

#16 Updated by intrigeri 2018-05-24 13:26:07

  • Target version changed from Tails_3.8 to Tails_3.9

(This won’t be needed for 3.8 since we won’t update the APT snapshots.)

#17 Updated by intrigeri 2018-06-26 06:39:39

  • Priority changed from Elevated to High

Since torbrowser-launcher 0.2.9-3~bpo9+1 made it into stretch-backports, this now breaks the build of our devel branch (assuming Bug #15622 is merged, ’cause that other one breaks the build earlier).

#18 Updated by intrigeri 2018-06-26 06:39:49

  • blocked by Bug #15622: Upgrade to Linux 4.16+ in Tails 3.9 added

#19 Updated by intrigeri 2018-06-27 08:32:40

  • blocks Bug #15638: Remove deb.torproject.org sid APT source added

#20 Updated by intrigeri 2018-06-27 08:44:51

  • blocks Bug #15116: X.Org does not start with some NVidia Maxwell and Pascal graphic cards added

#21 Updated by intrigeri 2018-06-27 09:04:49

  • % Done changed from 50 to 60

intrigeri wrote:
> intrigeri wrote:
> > anonym wrote:
> > > intrigeri wrote:
> > >> I’m not super happy with passing a non-path to the Onion Grater’s exe-paths.
> >
> > > Agreed!
> >
> > >> Shall we rename that variable?
> >
> > > Yes! I wonder how, though. The code needs a similar change since returning the AppArmor profile name doesn’t make sense in a function called exe_path_of_pid().
> >
> > I propose:
> >
> > * s/exe-paths/apparmor-profiles/
> > * s/exe_path_of_pid/apparmor_profile_of_pid/
> > * some more strings and the doc need adjustments
> >
> > Shall I go ahead?
>
> I’ll do this.

Done in commit:6ac1a731a4a3467b3ad03fcdf66890d9ff00ffd3. Will run the full test suite and test manually software that goes through Onion Grater but that’s not exercised in our automated test suite.

#22 Updated by intrigeri 2018-06-27 09:10:19

  • QA Check set to Ready for QA

#23 Updated by intrigeri 2018-06-28 13:18:12

  • blocks Bug #15627: feature/14594-asp-gui FTBFS due to merge conflicts added

#24 Updated by intrigeri 2018-06-28 13:23:21

  • % Done changed from 60 to 70

intrigeri wrote:
> Will run the full test suite

Done. Accross 3 runs, every scenario passed at least once.

Next step: test manually software that goes through Onion Grater but that’s not exercised in our automated test suite.

#25 Updated by intrigeri 2018-06-28 13:36:43

In /etc/onion-grater.d/ we have config for:

  • OnionCircuits: works fine
  • OnionShare: broken but I think that’s because of the upgrade to 1.3 (we’re currently pinning the package to version from sid, see Bug #14649) and unrelated to this branch
  • Tor Browser: new identity, circuits display, “new circuit for this website” all work fine
  • Tor Launcher: already exercised by automated test suite

#26 Updated by intrigeri 2018-06-28 15:08:31

  • Assignee changed from intrigeri to segfault

Allright, OnionShare fixed by installing the same version as in Tails 3.8. Please do a code review (based on the one anonym did already, see above). No need to replicate the automated test suite + my manual tests but if you can think of something I forgot to exercise, by all means, please test it. All this should fit in 20 minutes.

#27 Updated by intrigeri 2018-06-28 16:33:28

  • blocks Feature #8514: Replace WhisperBack.mail_appended_info with a dictionary added

#28 Updated by intrigeri 2018-06-29 13:11:28

  • blocks Bug #15690: Stop installing all "Priority: standard" packages only to remove some of them later added

#29 Updated by segfault 2018-06-29 13:12:29

  • Assignee changed from segfault to intrigeri
  • QA Check changed from Ready for QA to Pass

I had a hard time reviewing the diff of the diffs in the patch file, but eventually came to the conclusion that it does what it’s supposed to do.

#30 Updated by intrigeri 2018-06-29 13:29:32

segfault wrote:
> I had a hard time reviewing the diff of the diffs in the patch file, but eventually came to the conclusion that it does what it’s supposed to do.

Indeed, understood… which is why there’s torbrowser-launcher:feature/15472-torbrowser-apparmor-updates in the feature branch field. So another way to review this would have been to review the Git changes in there and then check that the patch I’ve exported from there into tails.git matches these changes. I guess I could have made this explicit right from the beginning, sorry! We’ll do better next time.

Anyway, thank you so much for the prompt review => merging.

#31 Updated by segfault 2018-06-29 13:31:30

> So another way to review this would have been to review the Git changes in there and then check that the patch I’ve exported from there into tails.git matches these changes.

That’s what I did in the end :)

#32 Updated by intrigeri 2018-06-29 13:34:50

  • Status changed from In Progress to Fix committed
  • Assignee deleted (intrigeri)
  • % Done changed from 70 to 100

Merged into the 2 repos.

#33 Updated by intrigeri 2018-06-29 13:35:11

segfault wrote:
> > So another way to review this would have been to review the Git changes in there and then check that the patch I’ve exported from there into tails.git matches these changes.
>
> That’s what I did in the end :)

:)))

#34 Updated by Anonymous 2018-08-18 10:37:09

  • related to Feature #10422: Grant Tor Browser access to files as designated by the user added

#35 Updated by intrigeri 2018-09-05 16:08:59

  • Status changed from Fix committed to Resolved