833236 - [FUGU][wifi]It is shown as WAPI PSK type when searching and connecting to the AP which is set as WAP2 PSK.(617001975709)

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Wifi, defect)

Description

[Firefox_Mozilla]

Steps to reproduce:
1. set WIFI server as WAPI PSK authentication type;
2. go into wifi and search the AP of this server.

Expected results:
The access point encryption is WPA2 PSK.

Actual results:
the encryption of the access point is recongnized as WPA-PSK

Comment 1

[Vincent Chang[:vchang][changyihsin]]

(In reply to Firefox_Mozilla from comment #0)
> Steps to reproduce:
> 1. set WIFI server as WAPI PSK authentication type;
> 2. go into wifi and search the AP of this server.
> 
> Expected results:
> The access point encryption is WPA2 PSK.
> 
> Actual results:
> the encryption of the access point is recongnized as WPA-PSK
Does the wpa_supplicant and wifi driver support WAPI PSK ? 
First of all, I would like to have the AP which supports WAPI PSK.

Comment 2

[Vincent Chang[:vchang][changyihsin]]

This is a new feature and needs the support from wpa_supplicant and wifi driver. 
Not sure if our Unagi Android supports this feature or not. 
Because the wpa_supplicant and Wifi driver is provided by vendor, can you help to double confirm with them ? Also do we have the plan to support this feature ?

Comment 3

[Kai-Chih Hu [:khu]]

Good point. [e-mail], do you have the answer for Vincent's question?

Comment 4

[Firefox_Mozilla]

I confirm the wpa_supplicant and wifi driver support WAPI PSK. Other Android phone can identify it as WPA-PSK.

Comment 5

[Kai-Chih Hu [:khu]]

Let's do it after China market is included & planned.

Comment 6

[Vincent Chang[:vchang][changyihsin]]

Drop it for now because of comment 5.

Comment 7

[gbennett]

I was able to repro this issue on the following builds/devices.

Environmental Variables
Device: Buri 1.2 mozRIL
Build ID: 20130916040205
Gecko: http://hg.mozilla.org/mozilla-central/rev/c4bcef90cef9
Gaia: a0079597d510ce8ea0b9cbb02c506030510b9eeb
Platform Version: 26.0a1

Environmental Variables
Device: Leo 1.1 comRIL
Build ID: 20130916041201
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/bebaca30ea30
Gaia: 8b20ff956f8e366a17a7ef5f64935a6a6a8cb0c0
Platform Version: 18.1
RIL Version: 01.01.00.019.225

Comment 8

[Pin Zhang [:pzhang] (inactive)]

I'm working with Sam@SPRD on this issue.

Comment 9

[Pin Zhang [:pzhang] (inactive)]

Attachment: https://github.com/mozilla-b2g/gaia/pull/14735

Comment 10

[Pin Zhang [:pzhang] (inactive)]

Attachment: gecko.patch

Comment 11

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 8348492 [details] [diff] [review]
gecko.patch

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

Thank you for jumping to this. WAPI-PSK is a proprietary security type for China market.  
I don't have the Wifi AP that support WAPI-PSK security type here. 
Please make sure that you have verified the patch on real device, and it doesn't break the normal function. :-)
Please address my comment and post a new patch.

::: dom/wifi/WifiWorker.js
@@ +67,4 @@
>  const WIFI_SECURITY_TYPE_NONE     = "open";
>  const WIFI_SECURITY_TYPE_WPA_PSK  = "wpa-psk";
>  const WIFI_SECURITY_TYPE_WPA2_PSK = "wpa2-psk";
> +const WIFI_SECURITY_TYPE_WAPI_PSK = "wapi-psk";

I don't think we need this. It's for wifi hotspot.

@@ +2572,1 @@
>  

Why do you need to check the WAPI-PSK here ? This function is for wifi hotspot. Do we also support WAPI-PSK in wifi hotspot mode ?

Comment 12

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8348491 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/14735

r=me for the settings part. Thanks for the patch, Ping.

Flag Borja for the FTU part.

Comment 13

[Pin Zhang [:pzhang] (inactive)]

Attachment: Patch v2

Addressed

Comment 14

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 8351164 [details] [diff] [review]
Patch v2

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

Looks good. Thank you.

Comment 15

[Pin Zhang [:pzhang] (inactive)]

Hi Borja, could you help to review the FTU part? Thanks.

Comment 16

[Borja Salguero [:borjasalguero]]

Just arrived from Holidays! Taking a look :)

Comment 18

[David Palomino [:dpalomino]]

Hi, 

This issue was also reported with open device (adding dep to meta bug). We think this it'd be important to be solved for 1.3. Nominating it for discussion

Comment 19

[Preeti Raghunath(:Preeti)]

moving to minus since we shipped 1.1/1.2 with this

Comment 20

[Pin Zhang [:pzhang] (inactive)]

Attachment: Patch v2 rebased

Here is the rebased path, Gaia PR is also rebased on top of the master branch.

Comment 23

[Pin Zhang [:pzhang] (inactive)]

(In reply to Borja Salguero [:borjasalguero] from comment #16)
> Just arrived from Holidays! Taking a look :)

Hi Borja, any update?

Comment 24

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

(In reply to Arthur Chen [:arthurcc] from comment #12)
> Comment on attachment 8348491 [details] [review]
> https://github.com/mozilla-b2g/gaia/pull/14735
> 
> r=me for the settings part. Thanks for the patch, Ping.
> 
> Flag Borja for the FTU part.

I think we don't need the ASCII/HEX select.
At least in WPA-PSK, when string length is 8 to 63, it's ASCII; when string length is 64, it's hex.
is there a different rule in WAPI?

Also I think the hex to char transform should be done in wpa_supplicant, at least it's how WPA-PSK does.
At least it should be moved into gecko so others who don't know/can't use WifiHelper can set passphrase correctly.

Another concern is config values like "key_mgmt=WAPI-PSK" and "proto=WAPI" are proprietary values defined by vendor-specific wpa_supplicant.
What's our policy about supporting vendor-specific functions in master repo?

Comment 25

[Pin Zhang [:pzhang] (inactive)]

(In reply to Chuck Lee [:chucklee] from comment #24)
> (In reply to Arthur Chen [:arthurcc] from comment #12)
> > Comment on attachment 8348491 [details] [review]
> > https://github.com/mozilla-b2g/gaia/pull/14735
> > 
> > r=me for the settings part. Thanks for the patch, Ping.
> > 
> > Flag Borja for the FTU part.
> 
> I think we don't need the ASCII/HEX select.
> At least in WPA-PSK, when string length is 8 to 63, it's ASCII; when string
> length is 64, it's hex.
> is there a different rule in WAPI?
The rule of the password length is almost the same for HEX and ASCII, except the length of the HEX password must be even. 

> 
> Also I think the hex to char transform should be done in wpa_supplicant, at
> least it's how WPA-PSK does.

FUGU did transformation in their wpa_supplicant, but requires a proprietary configure key (psk_key_type), so I decided to implement it in Gaia.

> At least it should be moved into gecko so others who don't know/can't use
> WifiHelper can set passphrase correctly.

I we did this in Gecko, I think we need to modify the API to pass the type of the password, i.e. HEX or ASCII.

> 
> Another concern is config values like "key_mgmt=WAPI-PSK" and "proto=WAPI"
> are proprietary values defined by vendor-specific wpa_supplicant.
> What's our policy about supporting vendor-specific functions in master repo?

Sorry, i don't know, but take FM radio for example, GonkFMRadio.cpp only works for Qualcom HW, FUGU add their own GonkFMRadio implementation, so I think we could file another bug for this, and provide the solution when we have some other devices to support, what do you think?

Comment 26

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

(In reply to Pin Zhang [:pzhang] from comment #25)
> > At least it should be moved into gecko so others who don't know/can't use
> > WifiHelper can set passphrase correctly.
> 
> I we did this in Gecko, I think we need to modify the API to pass the type
> of the password, i.e. HEX or ASCII.

In such case, I would prefer:
In Gaia, set |password.type| to "ASCII" or "HEX" to |wifiHelper.setPassword()|, so we don't have to change the function definition, then the network parameter can be passed to |WifiManager.associate()| as normal.
In Gecko, do the psk transform in |netFromDOM()|.

> > 
> > Another concern is config values like "key_mgmt=WAPI-PSK" and "proto=WAPI"
> > are proprietary values defined by vendor-specific wpa_supplicant.
> > What's our policy about supporting vendor-specific functions in master repo?
> 
> Sorry, i don't know, but take FM radio for example, GonkFMRadio.cpp only
> works for Qualcom HW, FUGU add their own GonkFMRadio implementation, so I
> think we could file another bug for this, and provide the solution when we
> have some other devices to support, what do you think?

Changing FM Radio implementation only affects hal and nothing else, which is hal expected to do.
But supporting WAPI here affects Gecko and Gaia, makes them platform-dependent.
I personally like to leave these patches in partners' own code base.

Comment 27

[Pin Zhang [:pzhang] (inactive)]

(In reply to Chuck Lee [:chucklee] from comment #26)
> (In reply to Pin Zhang [:pzhang] from comment #25)
> > > At least it should be moved into gecko so others who don't know/can't use
> > > WifiHelper can set passphrase correctly.
> > 
> > I we did this in Gecko, I think we need to modify the API to pass the type
> > of the password, i.e. HEX or ASCII.
> 
> In such case, I would prefer:
> In Gaia, set |password.type| to "ASCII" or "HEX" to
> |wifiHelper.setPassword()|, so we don't have to change the function
> definition, then the network parameter can be passed to
> |WifiManager.associate()| as normal.
> In Gecko, do the psk transform in |netFromDOM()|.

It might works, let me take a look.

> 
> > > 
> > > Another concern is config values like "key_mgmt=WAPI-PSK" and "proto=WAPI"
> > > are proprietary values defined by vendor-specific wpa_supplicant.
> > > What's our policy about supporting vendor-specific functions in master repo?
> > 
> > Sorry, i don't know, but take FM radio for example, GonkFMRadio.cpp only
> > works for Qualcom HW, FUGU add their own GonkFMRadio implementation, so I
> > think we could file another bug for this, and provide the solution when we
> > have some other devices to support, what do you think?
> 
> Changing FM Radio implementation only affects hal and nothing else, which is
> hal expected to do.
> But supporting WAPI here affects Gecko and Gaia, makes them
> platform-dependent.
> I personally like to leave these patches in partners' own code base.

The WAPI feature must be supported in any devices that be saled in China, so these codes should be maintained in our codebase, and the need-to-be-configured-by-parteners part is very mininal, e.g. the configured values, I think the case is the same as FM radio gonk for which we defined the hal interface that FMRadio API needed, and here what we need to do is defined the wapi-config-interface for parteners.

Comment 28

[Pin Zhang [:pzhang] (inactive)]

Attachment: Convert HEX to ASCII in Gecko

The is the incremental patch based on attachment 8361461 [details] [diff] [review].

Comment 29

[Pin Zhang [:pzhang] (inactive)]

Attachment: Do not convert in Gaia

This is an incremental patch based on attachment 8348491 [details] [review].

Comment 30

[Pin Zhang [:pzhang] (inactive)]

Attachment: Do not convert in Gaia

Comment 31

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Comment on attachment 8375400 [details] [diff] [review]
Do not convert in Gaia

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

Looks good, thanks!

Please also update the Gaia pull request.

Comment 32

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Comment on attachment 8375398 [details] [diff] [review]
Convert HEX to ASCII in Gecko

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

Please add a check on HEX psk before transforming it, others looks good!

::: dom/wifi/WifiWorker.js
@@ +1627,5 @@
> +    if (net.key_mgmt == "WAPI-PSK") {
> +      net.proto = configured.proto = "WAPI";
> +
> +      // convert HEX to ASCII
> +      if (net.pskType == "HEX") {

I think we also have to validate the hex string from DOM, but netFromDOM can't return error with message now.
We can just skip the transform on invalid hex string here(and connection will fail due to incorrect password), and file a follow up bug to report error in netToDOM/netFromDOM.

Comment 33

[Pin Zhang [:pzhang] (inactive)]

Attachment: Convert HEX to ASCII in Gecko v2

Comment 34

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Comment on attachment 8376008 [details] [diff] [review]
Convert HEX to ASCII in Gecko v2

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

Thank you!

Comment 35

[Pin Zhang [:pzhang] (inactive)]

Gaia PR is updated.(In reply to Chuck Lee [:chucklee] from comment #34)
> Comment on attachment 8376008 [details] [diff] [review]
> Convert HEX to ASCII in Gecko v2
> 
> Review of attachment 8376008 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you!

Gaia PR has been updated.

Comment 36

[Pin Zhang [:pzhang] (inactive)]

Attachment: Gecko full patch rebased

This is the full patch for Gecko rebased on top of latest commit, Gaia PR is also rebased.

Comment 37

[Pin Zhang [:pzhang] (inactive)]

(In reply to Pin Zhang [:pzhang] from comment #23)
> (In reply to Borja Salguero [:borjasalguero] from comment #16)
> > Just arrived from Holidays! Taking a look :)
> 
> Hi Borja, any update?

Hi Borja, do you have bandwidth for reviewing the gaia patch? It's OK if you are overloaded right now, I can ask someone else for help.

Comment 38

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

There's another discussion about implementing WAPI per bug 953237 comment 21.

Comment 39

[Borja Salguero [:borjasalguero]]

Comment on attachment 8348491 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/14735

Removing flag due to https://bugzilla.mozilla.org/show_bug.cgi?id=833236#c38

Comment 40

[BMO Automation]

Firefox OS is not being worked on