736089 - Wifi: Handle broken wifi configurations more gracefully

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect, P1)

Description

[Blake Kaplan (:mrbkap) (inactive)]

Currently, if you mis-enter a password or try to connect to a mis-configured network, wpa_supplicant loops until user interaction. We should:

a) Detect when we're looping through the authentication phases and automatically disable the network after a few tries.
b) Catch "invalid password" type messages from the supplicant and fire events

Comment 1

[Dietrich Ayala (:dietrich)]

Sounds like it results in a state in which no connection occurs and the user is not notified, which is not OK. Blocking+.

Comment 2

[Tony Chung [:tchung]]

crossposting gaia bug: https://github.com/mozilla-b2g/gaia/issues/2768

Comment 3

[Kai-Chih Hu [:khu]]

Should it be handled by network manager and then deployed the error to apps?

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

khu: I don't understand what you mean, exactly. The network manager is only notified when interfaces go up and down and for connection/disconnection events. It doesn't handle things at a low enough level.

That being said, there are a few things that need to happen here:
  - wpa_supplicant will sometimes send error messages (right now we have an internal message for "pre-shared key might be incorrect"). In that case, we should notify the DOM side via a notification and cancel the connection attempt (disable the network).
  - Sometimes (especially with a weak signal, or with some WPA/WEP networks) there won't be any notification, but we'll associate and then fail to get DHCP, or we'll go from one of the connecting states earlier than "COMPLETED" repeatedly. We need to detect when we're looping between these states and abandon the connection attempt.

One tricky bit will be to figure out when to re-enable disabled networks in a way that doesn't knock off a lower-priority but established connection.

Comment 5

[Andreas Gal :gal]

How are we doing here?

Comment 6

[Vincent Chang[:vchang][changyihsin]]

(In reply to Blake Kaplan (:mrbkap) from comment #4)
> khu: I don't understand what you mean, exactly. The network manager is only
> notified when interfaces go up and down and for connection/disconnection
> events. It doesn't handle things at a low enough level.
> 
> That being said, there are a few things that need to happen here:
>   - wpa_supplicant will sometimes send error messages (right now we have an
> internal message for "pre-shared key might be incorrect"). In that case, we
> should notify the DOM side via a notification and cancel the connection
> attempt (disable the network).

  We can have a counter and increase the count when receive this error message. If the counter is bigger than certain value in DISCONNECTED state, disable the current network and notify DOM side. 

>   - Sometimes (especially with a weak signal, or with some WPA/WEP networks)
> there won't be any notification, but we'll associate and then fail to get
> DHCP, or we'll go from one of the connecting states earlier than "COMPLETED"
> repeatedly. We need to detect when we're looping between these states and
> abandon the connection attempt.

  wpa_supplicant may send these messages "AUTHENTICATING, ASSOCIATING, ASSOCIATED, FOUR_WAY_HANDSHAKE, GROUP_HANDSHAKE" to our event worker. When we receive one of these message, we are in handshake state. If we receive one of these message continually(more than four times) in handshake state, then we are in a loop. We have to disable the current network and notify DOM side.   
> 
> One tricky bit will be to figure out when to re-enable disabled networks in
> a way that doesn't knock off a lower-priority but established connection.
Do we need to disconnect a lower-priority Ap and re-connect to higher priority here ?

Comment 7

[Vincent Chang[:vchang][changyihsin]]

Attachment: WIP

Fire the connection fail event to DOM site in case of 
1. pre-shared key may be incorrect
2. detect wpa_supplicant loop in handshake state.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 652695 [details] [diff] [review]
WIP

Review of attachment 652695 [details] [diff] [review]:
-----------------------------------------------------------------

This looks basically right (so f+) but there are a couple of non-nit comments.

::: dom/wifi/WifiWorker.js
@@ +1224,5 @@
> +    var isStateInHandShake = manager.isHandShakeState(state);
> +
> +    if (isPrevStateInHandShake) {
> +      // Increase the count if we are already in HandShake state.
> +      if (isStateInHandShake) {

I think this will increase loopDetectionCount too quickly. IMO we're only looping if we go e.g. from a later state to an earlier state. It's a little annoying that internally our states are simply strings, since it makes that comparison harder to do, but I think we have to do something like that.

@@ +1586,5 @@
>                                    netId: WifiManager.connectionInfo.id };
>            WifiManager.getNetworkConfiguration(self.currentNetwork, function(){});
>          }
> +        // The full authentication process is completed, reset the count.
> +        WifiManager.authenticationFailuresCount = 0;

Do you have to set loopDetectionCount to 0 here as well?

Comment 9

[Vincent Chang[:vchang][changyihsin]]

> I think this will increase loopDetectionCount too quickly. IMO we're only
> looping if we go e.g. from a later state to an earlier state. It's a little
> annoying that internally our states are simply strings, since it makes that
> comparison harder to do, but I think we have to do something like that.

loopDetectionCount will be increased by 1 when previous state is in handshake state and current state is also in handshake state. And reset to zero when first time move from other states to handshake state.  
"previous state" "current state" "loopdetectcount" 
0                0               0
0                1               0
1                1               1
1                1               2
1                0               2 
0                0               2
0                1               0 
          
> > +        // The full authentication process is completed, reset the count.
> > +        WifiManager.authenticationFailuresCount = 0;
> 
> Do you have to set loopDetectionCount to 0 here as well?

Because the logical above can help to reset loopDetectionCount, we may not have to do it here. But can be safer if set it to zero as well.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Vincent Chang from comment #9)
> loopDetectionCount will be increased by 1 when previous state is in
> handshake state and current state is also in handshake state.

This feels counterintuitive to me. We're not looping until we go from a later state to an earlier one. The way this is described, the number of loops that are possible will depend on the type of security used (more handshake states means fewer loops). I'd really prefer to see "loop" actually meaning loop.

> Because the logical above can help to reset loopDetectionCount, we may not
> have to do it here. But can be safer if set it to zero as well.

Ah, right. I'd still prefer to explicitly reset it for clarity and explicitness (also since it parallels authenticationFailuresCount.

Comment 11

[Vincent Chang[:vchang][changyihsin]]

Attachment: Updated the patch V2

Updated the patch based on comment 8 and 10.

Comment 12

[Vincent Chang[:vchang][changyihsin]]

Attachment: Updated the patch V2.1

1. set the current network to null 
2. restore one blank line

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 656147 [details] [diff] [review]
Updated the patch V2.1

r=me. Thanks a lot!

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

https://hg.mozilla.org/mozilla-central/rev/a0240c1043ee