Project

General

Profile

Feature #5315

Warn users when running in a non-free VM

Added by Tails over 5 years ago. Updated about 3 years ago.

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

100%

QA Check:
Pass
Feature Branch:
Type of work:
Code
Blueprint:
Starter:
No
Affected tool:

Description

When running inside a non-free VM, suggest that the user switches to VirtualBox or some other free software VM.

Among the "facts" reported by virt-what(1), these ones are free: kvm, qemu, uml, virtualbox, xen, xen-domU, xen-hvm. So, if one of the lines returned by virt-what is not in this set, then we should assume Tails is running inside a non-free virtualization environment.

The file where this logics should be implemented is /usr/local/bin/tails-virt-notify-user.

updated-warning.txt View (2.46 KB) hybridwipe, 08/10/2015 01:04 PM

Associated revisions

Revision 40469a92 (diff)
Added by intrigeri about 3 years ago

tails-virt-notify-user: notify the user if running in a non-free vm

Thanks to Austin English <> for the patch.

Sorry, my `git am' does not want to hear about this patch,
so I'm applying it by hand.

Closes: #5315

History

#1 Updated by intrigeri about 5 years ago

  • Priority changed from Normal to Low
  • Starter set to No

#2 Updated by BitingBird over 4 years ago

  • Subject changed from warn users when running in a non-free VM to Warn users when running in a non-free VM

#3 Updated by sajolida almost 4 years ago

  • Category set to Virtualization

#4 Updated by hybridwipe over 3 years ago

  • File 0001-tails-virt-notify-user-notify-user-if-they-re-runnin.patch added

I've attached a patch to implement this. It also converts the patch to a (IMHO much more readable) shell script.

Please review and let me know of any issues. Thanks in advance.

#5 Updated by intrigeri over 3 years ago

  • Assignee set to intrigeri
  • Target version set to Tails_1.5
  • QA Check set to Ready for QA

I've attached a patch to implement this.

Thanks! I'll look at it shortly.

#6 Updated by BitingBird over 3 years ago

  • Status changed from Confirmed to In Progress
  • % Done changed from 0 to 20

#7 Updated by intrigeri over 3 years ago

  • Assignee changed from intrigeri to hybridwipe
  • QA Check changed from Ready for QA to Dev Needed

So, I've taken a look, and fundamentally it looks pretty good. Here are a few improvement suggestions.

  • I could personally be fine with the move to shell scripting, but the bad news is that this will break when porting to Jessie (#7989); see the changes that were made to this script in our feature/jessie branch, that introduce notification actions to react when the user clicks the link. If you find a way to solve this problem in shell, fine (and actually it could be reused elsewhere, which would be awesome). Otherwise, I think we'll have to go back to Perl or Python.
  • If we stick to shell, why use bash instead of a POSIX shell interpreter?
  • If we stick to shell, please consider adding set -u, and dropping set -x.
  • The i18n support was dropped while converting to shell. If we stick to shell, please reintroduce it (see examples listed in SHELL_PROGS) and update refresh-translations.

Thanks again! :)

#8 Updated by hybridwipe over 3 years ago

  • File tails-virt-nonfree-notify-user-add-a-script-service.txt added

I attempted to add a second notification to the original tails-virt-notify-user script, but ran into concurrency issues; elected in the end to have a separate script, which looks much cleaner imo.

#9 Updated by intrigeri over 3 years ago

I attempted to add a second notification to the original tails-virt-notify-user script, but ran into concurrency issues; elected in the end to have a separate script, which looks much cleaner imo.

Why not instead display a single notification when running in a VM, with a slightly different text depending on whether the VM technology being used is free or not?

#10 Updated by hybridwipe over 3 years ago

  • File tails-notify-nonfree.txt added

Sure, good point.

#11 Updated by intrigeri over 3 years ago

  • QA Check changed from Dev Needed to Ready for QA

#12 Updated by intrigeri over 3 years ago

  • Assignee deleted (hybridwipe)

#13 Updated by intrigeri over 3 years ago

  • Assignee set to intrigeri

#14 Updated by intrigeri over 3 years ago

  • Assignee changed from intrigeri to hybridwipe
  • Target version changed from Tails_1.5 to Tails_2.0
  • QA Check changed from Ready for QA to Dev Needed

Here are a few comments:

  • Yay, getting closer to something we can merge. Congrats! :)
  • This is based on current feature/jessie, so I'm setting the Target version accordingly.
  • It's a bit sad to run /usr/bin/systemd-detect-virt twice, once to get its exit code, and another time to get its stdout. You could instead do something like my $vm_type = capturex([0, 1], qw{/usr/bin/systemd-detect-virt});, and then exit 0 if $EXITVAL 1; -- see the doc for IPC::System::Simple for details.
  • The list of if/elsif for setting $whitelist could be replaced with an array containing the whitelist, and then instead of if ($whitelist 1) we would check if $vm_type is in the whitelist.
  • We should call $notify->create only once, that is outside of the if ($whitelist == 1) block. This requires declaring my ($summary, body); outside of the if block, then setting $summary's and $body's value in the if/else block, and then calling $notify->create.

#15 Updated by hybridwipe about 3 years ago

  • File almost.txt added
  • File fails.txt added

intrigeri wrote:

Here are a few comments:

Thanks for all the comments, perl is not my forte, but that helped immensely.

  • Yay, getting closer to something we can merge. Congrats! :)
  • This is based on current feature/jessie, so I'm setting the Target version accordingly.
  • It's a bit sad to run /usr/bin/systemd-detect-virt twice, once to get its exit code, and another time to get its stdout. You could instead do something like my $vm_type = capturex([0, 1], qw{/usr/bin/systemd-detect-virt});, and then exit 0 if $EXITVAL 1; -- see the doc for IPC::System::Simple for details.
  • The list of if/elsif for setting $whitelist could be replaced with an array containing the whitelist, and then instead of if ($whitelist 1) we would check if $vm_type is in the whitelist.

Done.

  • We should call $notify->create only once, that is outside of the if ($whitelist == 1) block. This requires declaring my ($summary, body); outside of the if block, then setting $summary's and $body's value in the if/else block, and then calling $notify->create.

I wasn't able to get that to work, not sure if I did something wrong. I've attached two patches (almost.txt / fails.txt). With $notify->create moved outside of the if block, the summary is missing. The body is still present, however..perhaps you can find some mistake I made, as I said, I'm not comfortable in perl, so may have missed something obvious.

Thanks again for the review/tips.

#16 Updated by hybridwipe about 3 years ago

  • File deleted (fails.txt)

#17 Updated by hybridwipe about 3 years ago

  • File deleted (almost.txt)

#18 Updated by hybridwipe about 3 years ago

  • File deleted (tails-notify-nonfree.txt)

#19 Updated by hybridwipe about 3 years ago

  • File deleted (tails-virt-nonfree-notify-user-add-a-script-service.txt)

#20 Updated by hybridwipe about 3 years ago

  • File deleted (0001-tails-virt-notify-user-notify-user-if-they-re-runnin.patch)

#21 Updated by hybridwipe about 3 years ago

  • File patch.txt added

Found the problem in last patch, working patch attached for review.

#22 Updated by BitingBird about 3 years ago

  • Assignee changed from hybridwipe to intrigeri
  • QA Check changed from Dev Needed to Ready for QA

A patch was submitted for review -> setting as ready for QA.

#23 Updated by intrigeri about 3 years ago

  • Assignee changed from intrigeri to hybridwipe
  • % Done changed from 20 to 60
  • QA Check changed from Ready for QA to Dev Needed

Thanks!

  • I would add bochs to the whitelist. Would this be wrong?
  • Perhaps we could pass --vm to systemd-detect-virt? (I realize that it's orthogonal to this ticket, but while we're at it... it could make the call a bit faster and decrease a bit the attack surface.)
  • I can't find any reference to xen-domU nor xen-hvm in the manpage, be it on Jessie or current sid. Are they really values that can be output by systemd-detect-virt?
  • May you please consult the (public mailing-list) for how best to phrase the new warning you're introducing? It seems quite good already, but e.g. I'm not sure about the "for defects" part.

Except these few nitpicks, the last version of this patch looks perfect to me :)

How about getting yourself a Git repo by the way? This would ease reviewing. If interested, ask .

#24 Updated by hybridwipe about 3 years ago

  • File foo.txt added

intrigeri wrote:

Thanks!

  • I would add bochs to the whitelist. Would this be wrong?

Agreed.

  • Perhaps we could pass --vm to systemd-detect-virt? (I realize that it's orthogonal to this ticket, but while we're at it... it could make the call a bit faster and decrease a bit the attack surface.)

Sure.

  • I can't find any reference to xen-domU nor xen-hvm in the manpage, be it on Jessie or current sid. Are they really values that can be output by systemd-detect-virt?

That was from virt-what, see http://linux.die.net/man/1/virt-what and the original bug summary. systemd-detect-virt does not appear to support them, however, so I've removed them.

  • May you please consult the (public mailing-list) for how best to phrase the new warning you're introducing? It seems quite good already, but e.g. I'm not sure about the "for defects" part.

Sent an RFC.

#25 Updated by hybridwipe about 3 years ago

  • File deleted (patch.txt)

#26 Updated by intrigeri about 3 years ago

  • Assignee changed from hybridwipe to intrigeri
  • QA Check changed from Dev Needed to Ready for QA

Sent an RFC.

Cool. I'll try to review your latest patch in the meantime, but this may have to wait a couple weeks (after DebConf).

#27 Updated by hybridwipe about 3 years ago

Updated warning after discussion on tails-ux.

#28 Updated by hybridwipe about 3 years ago

  • File deleted (foo.txt)

#29 Updated by intrigeri about 3 years ago

  • % Done changed from 60 to 70

Code review passes. I'll now build and test.

#30 Updated by intrigeri about 3 years ago

  • Status changed from In Progress to Resolved
  • % Done changed from 70 to 100

#31 Updated by intrigeri about 3 years ago

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

Excellent, merged!

Also available in: Atom PDF