831948 - WiFi Tethering should default to WPA2

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Settings, defect)

Description

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

WiFi Tethering should default to WPA. There is no reason the default should be wide open as setting up WPA is super simple.

Comment 1

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Attachment: Link to pull request

Link to a pull request

Comment 2

[Fabien Cazenave [:kaze]]

Comment on attachment 704598 [details] [review]
Link to pull request

Your pull request currently includes your patch for bug 797661, please fix this and r? me again.

Comment 3

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Attachment: Link to pull request

Fixed the commit. Is now just a fix for this specific bug. New pull request is attached.

Comment 4

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

I'm posting this while being tethered to my B2G phone with this patch applied. This works really well. (Although slow on Edge)

(I also have 797661 applied so I was given a nice to remember password to connect)

Comment 5

[Tony Chung [:tchung]]

tef-.  nice to have.

Comment 6

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Tony, any way i can change your mind? We think this is an important improvement.

Comment 7

[Fabien Cazenave [:kaze]]

Comment on attachment 704637 [details] [review]
Link to pull request

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: poor protection when tethering over wifi
Testing completed: manual
Risk to taking this patch (and alternatives if risky): low

Comment 8

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 704637 [details] [review]
Link to pull request

Nice to have, low risk patch - please go ahead with uplift to v1-train and v1.0.0

Comment 9

[Matthew N. [:MattN]]

Can we land this?

Comment 10

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Please land this. I'm on paternity leave, but :kaze can assist.

Comment 11

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Re-nom'ing this since it seems it was approved above, but then fell off the radar. Patch is ready and low risk. Literally just changing:

- "tethering.wifi.security.type": "open",
+ "tethering.wifi.security.type": "wpa2-psk",

in build/settings.py

If this isn't appropriate, please just let me know. Just want to make sure that this wasn't just accidentally overlooked.

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

This has approval, and should have been uplifted, but it's still not a shipping blocker so we'll track here for v1-train.  Our auto-uplift driver should catch this.

Comment 13

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

I'm not in B2G engineering so I am unassigning myself. Hopefully someone can include this patch.

Comment 14

[Stephanie Ouillon [:arroway] (needinfo me)]

Attachment: tethering_wpa2.patch

New pull request since settings.py has moved to settings.js:
https://github.com/mozilla-b2g/gaia/pull/14609

Comment 15

[Fabien Cazenave [:kaze]]

Comment on attachment 8346494 [details] [diff] [review]
tethering_wpa2.patch

Thanks Stéphanie. Next time, please provide a github PR so we can get the Travis results before merging.

I’m nowhere near a security expert, but maybe you’ll want to open another bug to have a less obvious password by default? I’m a bit worried that the user might think his connection is properly secured with WPA2, without having to set a proper password first… UX might help on this subject, too.

Comment 16

[Fabien Cazenave [:kaze]]

(In reply to Fabien Cazenave [:kaze] from comment #15)
> I’m nowhere near a security expert, but maybe you’ll want to open another
> bug to have a less obvious password by default?

I just saw bug 797661, please ignore this comment.

Comment 17

[Stephanie Ouillon [:arroway] (needinfo me)]

(In reply to Fabien Cazenave [:kaze] from comment #15)
> Comment on attachment 8346494 [details] [diff] [review]
> tethering_wpa2.patch
> 
> Thanks Stéphanie. Next time, please provide a github PR so we can get the
> Travis results before merging.
> 
I don't understand what you mean: I did provide a github PR at the same time as the patch (link in comment 14)?

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/47266e33ea9f48c61c6cc8697999ede8477fe20e

Comment 19

[John Ford [:jhford] CET/CEST Berlin Time]

This bug was partially uplifted.

Uplifted 47266e33ea9f48c61c6cc8697999ede8477fe20e to:
v1.3: 5f21afd617054297d07e5de8e6dfba3f73b6562f
v1.2: 4f1c06318ad7c0134e965de90a0a4a64e8eaa6e3

Commit 47266e33ea9f48c61c6cc8697999ede8477fe20e didn't uplift to branch v1-train

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Is this something that needs to land on v1.1 still? IIUC, v1.1.x is only taking security updates now.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Paul, do we need this on v1-train for v1.1.x still?

Comment 22

[Stephanie Ouillon [:arroway] (needinfo me)]

Ryan,if it can be uplifted to 1.1.x, that would be nice of course. Did it fail because the code is incompatible and I need to write another patch for this version.
As for the security updates, I'm not too familiar with the process usually: after discussing it with Paul, at the moment we think this bug is not critical enough to ship a security update to the partner.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Presumably there were merge conflicts with v1-train, so if this is wanted, you'll need a new patch. However, you'll probably want to get the "is it still wanted for a v1.1.x release?" question answered by Preeti or Bhavana first.

Comment 24

[bhavana bajaj [:bajaj]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #23)
> Presumably there were merge conflicts with v1-train, so if this is wanted,
> you'll need a new patch. However, you'll probably want to get the "is it
> still wanted for a v1.1.x release?" question answered by Preeti or Bhavana
> first.

We generally tend to uplift sec-high,sec-crit or a critical crash regression. For the rest, we go by the security team recommendation. Given comment #22 I think its ok to wontfix on 1.1.x.