Project

General

Profile

Bug #15472

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

Added by bertagaz over 1 year ago. Updated about 1 year ago.

Status:
Resolved
Priority:
High
Assignee:
-
Category:
-
Target version:
Start date:
03/28/2018
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:

Description

Apparmor patches fail to apply.


Related issues

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

Associated revisions

Revision 30b8aeb8 (diff)
Added by bertagaz over 1 year ago

Install torbrowser-launcher from stretch-backports.

0.2.9-2 entered sid and our patches fail to apply since then. Let just
try to fix that quickly.

Refs: #15472

Revision 08a2c251 (diff)
Added by intrigeri over 1 year ago

Temporarily install torbrowser-launcher from stretch-backports (refs: #15472)

Our Tor Browser AppArmor policy path conflicts with torbrowser-launcher
0.2.9-2.

Revision ea9df1d5 (diff)
Added by bertagaz over 1 year ago

Add forgotten change to install torbrowser-launcher from stretch-backports.

This should be reverted with 30b8aeb84fbfdef5aca46f28f6b379097bd5f689
once the apparmor profiles have been adapted to 0.2.9-2 from Sid.

Refs: #15472

Revision c3906247 (diff)
Added by intrigeri over 1 year ago

Rebase our Tor Browser AppArmor policy on top of torbrowser-launcher 0.2.9-2's (refs: #15472)

… and drop temporary workaround to install torbrowser-launcher 0.2.9-1.

Revision 62ada7e8 (diff)
Added by intrigeri over 1 year ago

Update Onion Grater's config for new Tor Browser AppArmor profile name (refs: #15472).

Instead of the old:

/usr/local/lib/tor-browser/firefox {}

… that profile now is:

{torbrowser_firefox_executable} = /usr/local/lib/tor-browser/firefox
profile torbrowser_firefox
{torbrowser_firefox_executable} {}

Let's let Onion Grater know about it. Now, of course the "exe-paths" setting
name feels a bit weird.

Revision 6ac1a731 (diff)
Added by intrigeri over 1 year ago

Onion Grater: adjust variables and function name to match current behaviour (refs: #15472).

This is a follow-up to 6b13c98159915cf89215a22e4eb795d787b9f349.
An AppArmor profile name can be e.g. "torbrowser_firefox", and not necessarily
the path of the executable that's confined.

Revision ae547542 (diff)
Added by intrigeri over 1 year ago

Enable the feature-15472-torbrowser-apparmor-updates APT overlay (refs: #15472).

Revision 57c05619
Added by intrigeri over 1 year ago

Merge branch 'feature/15472-torbrowser-apparmor-updates' into devel (Fix-committed: #15472)

History

#1 Updated by bertagaz over 1 year ago

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

Actually devel is supposed to target 3.9.

#2 Updated by bertagaz over 1 year ago

  • Status changed from Confirmed to In Progress

#3 Updated by intrigeri over 1 year ago

  • 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 over 1 year ago

#5 Updated by intrigeri over 1 year ago

  • Priority changed from Normal to Elevated

#6 Updated by intrigeri over 1 year ago

  • 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 over 1 year ago

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

#8 Updated by intrigeri over 1 year ago

  • 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 over 1 year ago

Oops, I had blindly assumed that the short-term fix pushed by bertagaz straight to devel (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 over 1 year ago

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 over 1 year ago

  • 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 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 over 1 year ago

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 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 over 1 year ago

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 over 1 year ago

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

#15 Updated by intrigeri over 1 year ago

  • 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 over 1 year ago

  • 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 over 1 year ago

  • 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 #15622 is merged, 'cause that other one breaks the build earlier).

#18 Updated by intrigeri over 1 year ago

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

#19 Updated by intrigeri over 1 year ago

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

#20 Updated by intrigeri over 1 year ago

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

#21 Updated by intrigeri over 1 year ago

  • % 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 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 over 1 year ago

  • QA Check set to Ready for QA

#23 Updated by intrigeri over 1 year ago

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

#24 Updated by intrigeri over 1 year ago

  • % 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 over 1 year ago

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 #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 over 1 year ago

  • 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 over 1 year ago

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

#28 Updated by intrigeri over 1 year ago

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

#29 Updated by segfault over 1 year ago

  • 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 over 1 year ago

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 over 1 year ago

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 over 1 year ago

  • 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 over 1 year ago

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 u about 1 year ago

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

#35 Updated by intrigeri about 1 year ago

  • Status changed from Fix committed to Resolved

Also available in: Atom PDF