Project

General

Profile

Feature #6302

Feature #6298: Write more automated tests

Write MAC spoofing tests

Added by bertagaz about 6 years ago. Updated almost 4 years ago.

Status:
Resolved
Priority:
Normal
Assignee:
-
Category:
Test suite
Target version:
Start date:
09/26/2013
Due date:
% Done:

100%

Feature Branch:
test/6302-mac-spoofing
Type of work:
Code
Blueprint:
Starter:
No
Affected tool:

Description

Once implemented in Tails, we'll have to write a test to verify it's working.


Related issues

Related to Tails - Feature #7546: Add MAC spoofing to the manual test suite Rejected 07/10/2014
Blocked by Tails - Feature #5421: Spoof MAC address Resolved 12/27/2013
Blocked by Tails - Bug #10160: MAC spoofing panic mode is broken Resolved 09/03/2015
Blocks Tails - Bug #10336: Update "blocked wireless device" trace for Jessie Rejected 10/04/2015
Blocked by Tails - Feature #6094: Test suite: background snapshot improvements Resolved 10/15/2015
Blocks Tails - Feature #10340: Automatically test the Greeter's Disable All Networking option Resolved 10/06/2015

Associated revisions

Revision ca1e4299 (diff)
Added by anonym about 4 years ago

Test last corner case of MAC spoofing panic mode.

Will-fix: #6302

Revision dfa1d43d
Added by intrigeri almost 4 years ago

Merge branch 'test/6302-mac-spoofing' into devel

Fix-committed: #6302

History

#1 Updated by bertagaz about 6 years ago

  • Subject changed from Macchanger test to Write macchanger tests

#2 Updated by intrigeri about 6 years ago

  • Target version set to Sustainability_M1

#3 Updated by intrigeri over 5 years ago

  • Subject changed from Write macchanger tests to Write MAC spoofing tests

#4 Updated by intrigeri over 5 years ago

  • Related to Feature #7546: Add MAC spoofing to the manual test suite added

#6 Updated by anonym almost 5 years ago

  • Target version changed from Sustainability_M1 to Tails_1.8

#7 Updated by anonym almost 5 years ago

We can simulate:

  • macchanger returning failure by replacing its binary with a symlink to /bin/false.
  • macchanger returning success but any way failing to actually change MAC address by replacing its binary with a symlink to /bin/true.

This should cover the failure modes we want to test.

#8 Updated by anonym almost 5 years ago

  • Assignee set to kytv
  • Target version changed from Tails_1.8 to Tails_1.6

#9 Updated by anonym over 4 years ago

  • Status changed from Confirmed to In Progress
  • Assignee changed from kytv to anonym

I did some work on this, which I'll try to rebase and split up to atomic commits.

#10 Updated by anonym over 4 years ago

  • Feature Branch set to test/6302-mac-spoofing

anonym wrote:

I did some work on this, which I'll try to rebase and split up to atomic commits.

Done. It definitely needs some more work, though.

#11 Updated by intrigeri over 4 years ago

anonym wrote:

We can simulate:

  • macchanger returning failure by replacing its binary with a symlink to /bin/false.
  • macchanger returning success but any way failing to actually change MAC address by replacing its binary with a symlink to /bin/true.

This should cover the failure modes we want to test.

Yes. And given the #9531 "debacle", I think s/can/must/ is appropriate.

#12 Updated by anonym about 4 years ago

#13 Updated by anonym about 4 years ago

  • Blocked by Bug #10160: MAC spoofing panic mode is broken added

#14 Updated by anonym about 4 years ago

  • Assignee changed from anonym to kytv
  • % Done changed from 0 to 50
  • QA Check set to Ready for QA

When running the test suite with this branch checked out (i.e. testing it :)) it's important to build the image from the same branch (which we generally require any way). I also threw in some drive-by refactoring/cleanup work that affects a large part of the test suite but it's mostly mechanic change which shouldn't be hard to review, and just require a full run to test; that's commit 1439180. I hope that's not an issue.

kytv, when reviewing, it would be great if you also had a look at #10160, whose fix this branch includes by necessity. Also, you may be interested in having a look at robust_notification_wait() from commit 113dbf7. Perhaps you've seen other places which may benefit from that more robust implementation (all places?).

#15 Updated by anonym about 4 years ago

I've also pushed a test for tails-restircted-network-detector. The approach is not ideal, but I do not see any alternative given that we cannot virtualize wireless hardware. I also worry how to port the test to feature/jessie since it uses systemd, but I haven't looked into it at all.

#16 Updated by kytv about 4 years ago

Doing a full run of the test suite with this branch.

#17 Updated by kytv about 4 years ago

Re-running the full test suite on this branch. I think the other run was successful but I want to make sure.

#18 Updated by kytv about 4 years ago

Review passed other than a small typo:

--- a/features/mac_spoofing.feature
+++ b/features/mac_spoofing.feature
@@ -20,7 +20,7 @@ Feature: Spoofing MAC addresses
     And the network device has its default MAC address configured
     And the real MAC address was leaked

-  Scenario: MAC address spoofing is successfull
+  Scenario: MAC address spoofing is successful
     When I log in to a new session
     And the Tails desktop is ready
     And Tor is ready

Testing ongoing.

#19 Updated by kytv about 4 years ago

  • Assignee changed from kytv to anonym
  • QA Check changed from Ready for QA to Dev Needed

After the code review and testing I'm happy with this branch other than the minor typo mentioned at #6302#note-18.

Re-assigning to anonym to fix the typo before this gets sent to intrigeri.

#20 Updated by anonym about 4 years ago

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

kytv wrote:

After the code review and testing I'm happy with this branch other than the minor typo mentioned at #6302#note-18.

Fixed, thanks!

Re-assigning to anonym to fix the typo before this gets sent to intrigeri.

Did it for you.

intigeri, I'm quite sure you'll have time with this, so I guess we should postpone to 1.7, right?

#21 Updated by intrigeri about 4 years ago

  • Target version changed from Tails_1.6 to Tails_1.7

#22 Updated by intrigeri about 4 years ago

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

Hi, FYI this branch doesn't merge cleanly into current stable.

#23 Updated by intrigeri about 4 years ago

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

#24 Updated by intrigeri about 4 years ago

  • Assignee changed from intrigeri to anonym
  • % Done changed from 50 to 60
  • QA Check changed from Ready for QA to Dev Needed

Great work. I'll nitpick quite a bit on code style below, since that's a biggish set of changes, and as usual I've put myself in the "what if I have to understand how this thing works to fix a serious bug in there, in a hurry, some day" mindset for reviewing ;)

  • mac_spoofing.feature lacks a description ("As a [...]")
  • I've found it very confusing to see "And no network devices are present" and later "Then 1 network device is present". Initially I understood that "present" = "plugged in the computer", so I was looking for understand how a network device had appeared. May you please improve the clarity of this aspect a bit?
  • It strikes me as odd to read "I simulate that" in Gherkin text: we're simulating a *load of stuff anyway and IMO part of the point is to hide this fact at the Gherkin abstraction level. Let's just describe the reality we want when we'll be using mocks and stuff anyway.
  • I really like the way you've improved code readability with the fancy new libs => thing :) More sugar!
  • "And disable MAC spoofing in Tails Greeter" needs a subject ("I"): the way it's written right now relies on a specific ordering between steps, which is no good cucumber style I think.
  • In for addr_type in ["nic_ipv4_addr", "nic_ipv6_addr"] do, it seems that the addr_type variable could be better named, no?
  • "The below log was recorded from Tails based on Debian Wheezy" wants a ticket for Jessie, blocked by this one. Perhaps the code comment could be more general, to express the fact that installing a new NM (say, from backports) requires the same update process. Not sure where we can list such checks we have to do on a regular basis, btw.
  • Reading how we use the mac_leaks variable I wonder why it's not a set.
  • I don't understand why host_real_mac is called this way.
  • Reading the next few lines, in libs = [libs] if libs.class != Array it seems like we could test only for having a "map" method if we want the code to be more flexible (and thus easier to change in the future).
  • I've pushed a fix for a typo in a code comment (showed up in the diff context).

This change seems to have been introduced as part of a conflict resolution today:

-       "xdotool set_desktop '#{desktop_number}'", user
+      "xdotool search --name '#{desktop_number}' windowactivate --sync",

... and I'm not sure it's correct. May you please double-check? Especially, def do_focus below uses execute_successfully diffently so I wonder if the conflict resolution was good. Not run any tests yet, though.

#25 Updated by anonym about 4 years ago

  • Blocks Bug #10336: Update "blocked wireless device" trace for Jessie added

#26 Updated by anonym about 4 years ago

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

intrigeri wrote:

Great work. I'll nitpick quite a bit on code style below, since that's a biggish set of changes, and as usual I've put myself in the "what if I have to understand how this thing works to fix a serious bug in there, in a hurry, some day" mindset for reviewing ;)

And I'm thankful for it! :)

  • mac_spoofing.feature lacks a description ("As a [...]")

1795261

  • I've found it very confusing to see "And no network devices are present" and later "Then 1 network device is present". Initially I understood that "present" = "plugged in the computer", so I was looking for understand how a network device had appeared. May you please improve the clarity of this aspect a bit?

Indeed, "device" is ambiguous. What about 1017550?

  • It strikes me as odd to read "I simulate that" in Gherkin text: we're simulating a *load of stuff anyway and IMO part of the point is to hide this fact at the Gherkin abstraction level. Let's just describe the reality we want when we'll be using mocks and stuff anyway.

Yeah, sure. :) afb3521

  • I really like the way you've improved code readability with the fancy new libs => thing :) More sugar!

Yay! For the record, I already talked about this with kytv, but if you ever feel like you want to add methods with optional parameters, please use the Hash trick instead. It has a lot of advantages.

  • "And disable MAC spoofing in Tails Greeter" needs a subject ("I"): the way it's written right now relies on a specific ordering between steps, which is no good cucumber style I think.

9be839e

  • In for addr_type in ["nic_ipv4_addr", "nic_ipv6_addr"] do, it seems that the addr_type variable could be better named, no?

29668da

  • "The below log was recorded from Tails based on Debian Wheezy" wants a ticket for Jessie, blocked by this one. Perhaps the code comment could be more general, to express the fact that installing a new NM (say, from backports) requires the same update process.

#10336 + 15da834

I'm not exactly happy with this approach but I just found out about mac80211_hwsim and filed ticket #10335.

Not sure where we can list such checks we have to do on a regular basis, btw.

No idea. :/

  • Reading how we use the mac_leaks variable I wonder why it's not a set.

3e6c30a

  • I don't understand why host_real_mac is called this way.

I'm not entirely sure what you mean, but perhaps it is fixed by fb826a4?

  • Reading the next few lines, in libs = [libs] if libs.class != Array it seems like we could test only for having a "map" method if we want the code to be more flexible (and thus easier to change in the future).

5d74859

  • I've pushed a fix for a typo in a code comment (showed up in the diff context).

Thanks!

This change seems to have been introduced as part of a conflict resolution today:

[...]

... and I'm not sure it's correct. May you please double-check? Especially, def do_focus below uses execute_successfully diffently so I wonder if the conflict resolution was good. Not run any tests yet, though.

You're completely right. Fixed in 0347e2a.

However, since this feature changes stuff all over the place, let's not merge it before #6094. Once #6094 is merged, I'll adapt this branch for the final review'n'merge.

#27 Updated by anonym about 4 years ago

  • Blocked by Feature #6094: Test suite: background snapshot improvements added

#28 Updated by intrigeri about 4 years ago

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

I'm not commenting on the changes that entirely fix my concerns. Thanks for doing them, great work! There's only one code blocker left below, and then I'll run the tests.

Not sure where we can list such checks we have to do on a regular basis, btw.

No idea. :/

Let's just s/XXX: Jessie/XXX: Stretch/ in code comments for the time being.
When I port Tails to a new Debian I generally git grep $next_codename.

  • Reading how we use the mac_leaks variable I wonder why it's not a set.

3e6c30a

Great, thanks.

No big deal, not a blocker, but: I'm no fan of storing temporary state still under construction in an instance variable, which this commit silently starts doing, though; sounds like suboptimal OOP design to me.

  • I don't understand why host_real_mac is called this way.

I'm not entirely sure what you mean, but perhaps it is fixed by fb826a4?

Sorry, I meant "named this way", more accurately than "called this way". Let me clarify: in the context of code that's about managing VMs, "host" implicitly refers to the host system, and then if I read host_real_mac somewhere I think it's about the host system's MAC. It's wrong, isn't it?

#29 Updated by anonym about 4 years ago

  • Blocks Feature #10340: Automatically test the Greeter's Disable All Networking option added

#30 Updated by anonym about 4 years ago

  • Assignee changed from anonym to kytv
  • QA Check changed from Dev Needed to Ready for QA

intrigeri wrote:

I'm not commenting on the changes that entirely fix my concerns. Thanks for doing them, great work! There's only one code blocker left below, and then I'll run the tests.

Run, run, run!

Actually I'm first assigning it to kytv for a test run (and a review of the commits pushed since his last look, in case intrigeri missed something; nitpicking time, kytv!). kytv, please reassign to intrigeri once you're done. With my RM hat on: preferably do this within a week since this ticket is blocking other tickets targeting 1.7.

Not sure where we can list such checks we have to do on a regular basis, btw.

No idea. :/

Let's just s/XXX: Jessie/XXX: Stretch/ in code comments for the time being.
When I port Tails to a new Debian I generally git grep $next_codename.

I think we have to to git grep -iE $current_codename|$next_codename; we sometimes have XXX: workaround for $current version. Then we'll find the current comment.

  • Reading how we use the mac_leaks variable I wonder why it's not a set.

3e6c30a

Great, thanks.

No big deal, not a blocker, but: I'm no fan of storing temporary state still under construction in an instance variable, which this commit silently starts doing, though; sounds like suboptimal OOP design to me.

Sure, but when it's done inside a constructor I think it's fine. I guess it's best to be consistent, though: 7da257c

  • I don't understand why host_real_mac is called this way.

I'm not entirely sure what you mean, but perhaps it is fixed by fb826a4?

Sorry, I meant "named this way", more accurately than "called this way". Let me clarify: in the context of code that's about managing VMs, "host" implicitly refers to the host system, and then if I read host_real_mac somewhere I think it's about the host system's MAC. It's wrong, isn't it?

Duh! You are completely right. Fixed in 507c93e.

#31 Updated by intrigeri about 4 years ago

7da257c
507c93e.

Reviewed both, looks good, thanks! Hopefully kytv will be done soon, so that I'm notified I should test this (right now I'm on the almighty #6094).

#32 Updated by anonym about 4 years ago

Now when #6094 has been merged, I've dealt with the massive amounts of conflict resolutions for this branch. Please have a close look on merge commit f31fba2768ef6f01510d6b25e9b6ac2b4c97c2fd.

#33 Updated by kytv almost 4 years ago

  • Assignee changed from kytv to intrigeri

Sorry for the delay. I'm happy with the branch.

#34 Updated by intrigeri almost 4 years ago

I had another very quick look at the code and it looks OK. I'll run the new cucumber feature, and the ones I see failing on Jenkins.

#35 Updated by intrigeri almost 4 years ago

  • Status changed from In Progress to Fix committed
  • % Done changed from 60 to 100

#36 Updated by intrigeri almost 4 years ago

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

Congrats :)

#37 Updated by anonym almost 4 years ago

  • Status changed from Fix committed to Resolved

Also available in: Atom PDF