Closed Bug 745468 Opened 12 years ago Closed 10 years ago

B2G Wifi: Support EAP-PEAP and EAP-TTLS without private key

Categories

(Firefox OS Graveyard :: Wifi, defect)

Product:
Firefox OS Graveyard
Component:
Wifi
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(feature-b2g:2.0, tracking-b2g:backlog)

Status:
RESOLVED FIXED
Milestone:
2.0 S1 (9may)
Project Flags:
feature-b2g 2.0
tracking-b2g backlog

People

(Reporter: gerard-majax, Assigned: chucklee)

References

Details

Attachments

(3 files, 20 obsolete files)

Boot2Gecko phone connected on eduroam
56.74 KB, image/png
Details
0002. Support WPA-EAP connection state. V4, r=vchang,mrbkap
6.49 KB, patch
Details | Diff | Splinter Review
0001. Support WPA-EAP configure parameters. V8. r=vchang,mrbkap
6.27 KB, patch
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:13.0) Gecko/20120411 Firefox/13.0a2
Build ID: 20120411042011

Steps to reproduce:

There is currently, at least on a GUI level, no support to specify credentials for a WPA-EAP-TLS WiFi Network. As far as I can tell, there is no support (configuration) for EAP: world-wide "eduroam" network, provided in many universities, is thus not available for example. I did not checked wpa_supplicant's build configuration, but enabling those features at its level should not be a problem, only having a GUI that allows to specify the correct configuration and maybe wpa_supplicant's access to the credentials.
I've been able to get network connectivity by uploading my certificates to /etc/wifi/ and then configure WPA Supplicant through wpa_cli (I also put it in wpa_supplicant.conf, but it did not worked, I did not investigated on this).

> add_network
> scan_results
bssid / frequency / signal level / flags / ssid
00:21:91:f6:57:11	2442	-48	[WPA-EAP-TKIP+CCMP][ESS]	wifiSSID
> set_network 0 ssid "wifiSSID"
OK
> set_network 0 proto WPA
OK
> set_network 0 key_mgmt WPA-EAP
OK
> set_network 0 pairwise TKIP
OK
> set_network 0 eap TLS 
OK
> set_network 0 identity "username"
OK
> set_network 0 ca_cert "/etc/wifi/CAfile.pem"
OK
> set_network 0 private_key "/etc/wifi/usercert.p12"
OK
> set_network 0 private_key_passwd "passphrase"
OK
> list_networks
network id / ssid / bssid / flags
0	wifiSSID	any	[DISABLED]
> enable_network 0
OK
<3>CTRL-EVENT-STATE-CHANGE id=-1 state=3 BSSID=00:00:00:00:00:00
<3>CTRL-EVENT-SCAN-RESULTS 
<3>Trying to associate with 00:11:22:f6:57:11 (SSID='wifiSSID' freq=2442 MHz)
<3>CTRL-EVENT-STATE-CHANGE id=-1 state=5 BSSID=00:11:22:f6:57:11
<3>CTRL-EVENT-STATE-CHANGE id=0 state=6 BSSID=00:11:22:f6:57:11
<3>Associated with 00:11:22:f6:57:11
<3>CTRL-EVENT-EAP-STARTED EAP authentication started
<3>CTRL-EVENT-EAP-PROPOSED-METHOD vendor=0 method=25 -> NAK
<3>CTRL-EVENT-EAP-PROPOSED-METHOD vendor=0 method=13
<3>CTRL-EVENT-EAP-METHOD EAP vendor 0 method 13 (TLS) selected
<3>CTRL-EVENT-EAP-PEER-CERT depth=1 subject='/C=FR/ST=France/L=Town/O=Orga/OU=WiFi/CN=wifiSSID/emailAddress=root@localhost'
<3>CTRL-EVENT-EAP-PEER-CERT depth=0 subject='/C=FR/ST=France/L=Town/O=Orga/OU=WiFi/CN=192.168.2.254/emailAddress=root@localhost'
<3>CTRL-EVENT-EAP-SUCCESS EAP authentication completed successfully
<3>CTRL-EVENT-STATE-CHANGE id=0 state=7 BSSID=00:00:00:00:00:00
<3>CTRL-EVENT-STATE-CHANGE id=0 state=8 BSSID=00:00:00:00:00:00
<3>WPA: Key negotiation completed with 00:11:22:f6:57:11 [PTK=TKIP GTK=TKIP]
<3>CTRL-EVENT-CONNECTED - Connection to 00:11:22:f6:57:11 completed (auth) [id=0 id_str=]
<3>CTRL-EVENT-STATE-CHANGE id=0 state=9 BSSID=00:00:00:00:00:00

After this, I'm able to connect to the internet :)
This is going to need frontend work to expose the options and backend work to actually use them. We support identity/password authentication currently but not identity/private-key authentication.
Blocks: b2g-wifi
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: No EAP TLS support for WiFi → B2G Wifi: No EAP TLS support for WiFi
Great, with some help of kaze@ I started hacking on it.
Attached patch Gaia support for EAP TLS (obsolete) — — Splinter Review 
This patch introduces EAP-TLS support in Gaia's WiFi settings.
Attached patch Gecko support for EAP TLS (obsolete) — — Splinter Review 
This patch introduces the backend part of the EAP-TLS support, allowing Gecko to configure WPA Supplicant with EAP-TLS provided in Gaia's WiFi settings.
Attached patch Gaia support for EAP PEAP (obsolete) — — Splinter Review 
Not yet tested, but it should be good.
Attached patch Gecko support for EAP PEAP (obsolete) — — Splinter Review 
Not yet tested, but it should be good.
Tracking changes in EAP authentification (STARTED/SUCCESS/FAILURE only for now) and sending events accordingly.
Using events fired from Gecko to track the EAP authentication status, we notify user of this status.
Adding the missing string for locales en-US and fr to display current EAP authentication state.
Comment on attachment 615152 [details] [diff] [review]
Gaia support for EAP TLS

Please submit gaia changes as pull requests on github. Bugzilla only tracks gecko changes.

It's also a good idea to define the sequence of your patches (part 1, 2, etc.), to version each part and obsolete older versions. That way it's obvious to everybody how to apply them.
Attachment #615152 - Attachment is obsolete: true
Attachment #615166 - Attachment is obsolete: true
Attachment #615170 - Attachment is obsolete: true
Thanks, I created an account on github and sent a pull request for my branch.
I/wpa_supplicant( 1247): wlan0: Trying to associate with 00:26:3e:25:f5:c2 (SSID='eduroam' freq=2412 MHz)
I/wpa_supplicant( 1247): wlan0: Associated with 00:26:3e:25:f5:c2
I/wpa_supplicant( 1247): wlan0: CTRL-EVENT-EAP-STARTED EAP authentication started
I/wpa_supplicant( 1247): wlan0: CTRL-EVENT-EAP-PROPOSED-METHOD vendor=0 method=25
E/wpa_supplicant( 1247): TLS: Unsupported Phase2 EAP method 'MSCHAPv2'
I/wpa_supplicant( 1247): wlan0: CTRL-EVENT-EAP-METHOD EAP vendor 0 method 25 (PEAP) selected
I/wpa_supplicant( 1247): wlan0: CTRL-EVENT-EAP-PEER-CERT depth=3 subject='/C=SE/O=AddTrust AB/OU=AddTrust External TTP Network/CN=AddTrust External CA Root'
I/wpa_supplicant( 1247): wlan0: CTRL-EVENT-EAP-PEER-CERT depth=3 subject='/C=SE/O=AddTrust AB/OU=AddTrust External TTP Network/CN=AddTrust External CA Root'
I/wpa_supplicant( 1247): wlan0: CTRL-EVENT-EAP-PEER-CERT depth=2 subject='/C=US/ST=UT/L=Salt Lake City/O=The USERTRUST Network/OU=http://www.usertrust.com/CN=UTN-USERFirst-Hardware'
I/wpa_supplicant( 1247): wlan0: CTRL-EVENT-EAP-PEER-CERT depth=1 subject='/C=NL/O=TERENA/CN=TERENA SSL CA'
I/wpa_supplicant( 1247): wlan0: CTRL-EVENT-EAP-PEER-CERT depth=0 subject='/C=FR/L=TOURS/O=UNIVERSITE DE TOURS FRANCOIS RABELAIS/CN=rad1.univ-tours.fr'
I/wpa_supplicant( 1247): EAP-MSCHAPV2: Authentication succeeded
I/wpa_supplicant( 1247): EAP-TLV: TLV Result - Success - EAP-TLV/Phase2 Completed
I/wpa_supplicant( 1247): wlan0: CTRL-EVENT-EAP-SUCCESS EAP authentication completed successfully
I/wpa_supplicant( 1247): wlan0: WPA: Key negotiation completed with 00:26:3e:25:f5:c2 [PTK=CCMP GTK=TKIP]
I/wpa_supplicant( 1247): wlan0: CTRL-EVENT-CONNECTED - Connection to 00:26:3e:25:f5:c2 completed (auth) [id=0 id_str=]
Comment on attachment 615153 [details] [diff] [review]
Gecko support for EAP TLS

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

Thanks for the patch... I have one major comment, though...

::: dom/wifi/WifiWorker.js
@@ +1186,5 @@
> +    if (net.eap) {
> +      configured.eap = net.eap;
> +      if (net.eap == "TLS") {
> +        configured.pairwise = net.pairwise;
> +        checkAssign("ca_cert", false);

I think we should avoid these names becoming part of the DOM API. For one thing, underscores class with the DOM's camelCase convention and for another with JS, we have the ability to be a little bit clearer. I'd also like to avoid tainting the DOM with wpa_supplicant-specific stuff.

I don't have any concrete proposals off the top of my head though. Any suggestions would be welcome.
(In reply to Blake Kaplan (:mrbkap) from comment #14)
> Comment on attachment 615153 [details] [diff] [review]
> Gecko support for EAP TLS
> 
> Review of attachment 615153 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the patch... I have one major comment, though...
> 
> ::: dom/wifi/WifiWorker.js
> @@ +1186,5 @@
> > +    if (net.eap) {
> > +      configured.eap = net.eap;
> > +      if (net.eap == "TLS") {
> > +        configured.pairwise = net.pairwise;
> > +        checkAssign("ca_cert", false);
> 
> I think we should avoid these names becoming part of the DOM API. For one
> thing, underscores class with the DOM's camelCase convention and for another
> with JS, we have the ability to be a little bit clearer. I'd also like to
> avoid tainting the DOM with wpa_supplicant-specific stuff.
> 
> I don't have any concrete proposals off the top of my head though. Any
> suggestions would be welcome.

Just curious. I don't see any *.idl changes here so I assume nothing is exposed to the DOM?
Component: General → DOM: Device Interfaces
Product: Boot2Gecko → Core
QA Contact: general → device-interfaces
Version: unspecified → Trunk
(In reply to Vivien Nicolas (:vingtetun) from comment #15)
> Just curious. I don't see any *.idl changes here so I assume nothing is
> exposed to the DOM?

The DOM-visible stuff is passed through jsval-based APIs, so there aren't any IDL changes needed to expose them.
(In reply to Blake Kaplan (:mrbkap) from comment #16)
> (In reply to Vivien Nicolas (:vingtetun) from comment #15)
> > Just curious. I don't see any *.idl changes here so I assume nothing is
> > exposed to the DOM?
> 
> The DOM-visible stuff is passed through jsval-based APIs, so there aren't
> any IDL changes needed to expose them.

Make sense and I definitively agree with on abstracting wpa_supplicant.

Is networkConfigurationFields the list of field exposed to the DOM?
If yes this means there is already a few '_' values that should be removed and this can be done in a followup right? Or are those just used internally?
(In reply to Vivien Nicolas (:vingtetun) from comment #17)
> Is networkConfigurationFields the list of field exposed to the DOM?
> If yes this means there is already a few '_' values that should be removed
> and this can be done in a followup right? Or are those just used internally?

Those are the list of wpa_supplicant variables that we support internally. However, when we take a "network" object from the DOM, we pass it to netFromDOM (http://hg.mozilla.org/mozilla-central/file/cd8b66649278/dom/wifi/WifiWorker.js#l1140) which takes a smaller list of properties off of the object and translates them.
(In reply to Blake Kaplan (:mrbkap) from comment #18)
> (In reply to Vivien Nicolas (:vingtetun) from comment #17)
> > Is networkConfigurationFields the list of field exposed to the DOM?
> > If yes this means there is already a few '_' values that should be removed
> > and this can be done in a followup right? Or are those just used internally?
> 
> Those are the list of wpa_supplicant variables that we support internally.
> However, when we take a "network" object from the DOM, we pass it to
> netFromDOM
> (http://hg.mozilla.org/mozilla-central/file/cd8b66649278/dom/wifi/WifiWorker.
> js#l1140) which take is a smaller list of properties off of the object and
> translates them.

So the missing part of the patch is this translation process?
(In reply to Vivien Nicolas (:vingtetun) from comment #19)
> So the missing part of the patch is this translation process?

Well, we need to decide what the DOM side looks like so we can translate from/to that. Right now, the patch simply exposes the wpa_supplicant names to the DOM.
Comment on attachment 615153 [details] [diff] [review]
Gecko support for EAP TLS

r- until we can figure out proper DOM names. I'll try to add another comment here later today with a proposal.
Attachment #615153 - Flags: review?(gal) → review-
(In reply to Blake Kaplan (:mrbkap) from comment #21)
> Comment on attachment 615153 [details] [diff] [review]
> Gecko support for EAP TLS
> 
> r- until we can figure out proper DOM names. I'll try to add another comment
> here later today with a proposal.

Any proposal?
Attachment #615158 - Flags: review?(gal) → review+
Any progress here? I rather go with bad names temporarily than no names.
As I said on GitHub, we planned to work on this topic next weekend with kaze, benefitting from the MozFR meeting that will be held in Paris.
Here are a few ideas; it's linked to the discussion we had with kaze and vingtetun about moving wifi config out of wpa_supplicant.conf:
settings:
 - key: network.wifi.enabled = true|false
 - key: network.wifi.ssids =
[
	"OpenNetwork": {
		protection: "plain",
 	},
 	"WEP_Protected": {
 		protection: "wep",
 		keys: [ "key1", "key2", "key3" ],
 	},
 	"WPA_Protected": {
 		protection: "wpa",
 		wpa: {
 			mode: "key",
 		},
 		key: "value", // psk
 	},
 	"WPA2_Protected": {
 		protection: "wpa2",
 		wpa: {
 			mode: "key",
 		},
 		key: "value", // psk
 	},
 	"EAP-TLS_Protected": {
 		protection: "wpa", // or "wpa2"
 		wpa: {
 			mode: "eap"
 		},
 		eap: {
 			scheme: "tls",
 			identity: "user@domain.tld",
 			ca: "/path/to/ca_cert.pem",
 			cert: "/path/to/user_cert.pem",
 			privkey: "/path/to/user_private_key.p12",
 			// do not store private key password in settings, retrieve from keystore
 			keystore: "id",
 		},
 	},
 	"EAP-PEAP_Protected": {
 		protection: "wpa", // or "wpa2"
 		wpa: {
 			mode: "eap"
 		},
 		eap: {
 			scheme: "peap",
 			identity: "user@domain.tld",
 			ca: "/path/to/ca_cert.pem",
 			cert: "/path/to/user_cert.pem",
 			// do not store private key/user password in settings, retrieve from keystore
 			keystore: "id2",
 		},
 	}
]
Version: Trunk → Other Branch
(In reply to Alexandre LISSY from comment #25)
> Here are a few ideas; it's linked to the discussion we had with kaze and
> vingtetun about moving wifi config out of wpa_supplicant.conf:

This looks exactly right to me (whether or not we use wpa_supplicant.conf or not).
Blocks: 775293
It's been a long time for this one, maybe we could work on this now?
For the certification part, the backend work has already started on bug 867899 and bug 789217.
The plan is import certification files into NSS DB(bug 867899), and replace android's keystore daemon with keystore in gecko with protocol compatibility(bug 789217).
So wpa_supplicant can get certification from NSS without any modification, just use special prefix in config like
ca_cert="keystore://ID_OF_CERTIFICATION"

For the config part, it's on bug 786741.

Lastly, since we already have non-camel names in wifi config in DOM, and we have to modify wifi worker to support changing config name. I think we should fire another bug for this.
Assignee: nobody → chulee
blocking-b2g: --- → 1.3+
Chuck is handling WPA-EAP now.
Target Milestone: --- → 1.3 Sprint 4 - 11/8
Whiteboard: [FT:RIL]
Target Milestone: 1.3 Sprint 4 - 11/8 → 1.3 Sprint 5 - 11/22
Target Milestone: 1.3 Sprint 5 - 11/22 → 1.3 Sprint 6 - 12/6
Comment on attachment 615157 [details] [diff] [review]
Gaia support for EAP PEAP

Gaia Part will be handled in bug 926334, thanks for the patch here.
Attachment #615157 - Attachment is obsolete: true

        Attachment #615153 -
        Attachment is obsolete: true

        Attachment #615158 -
        Attachment is obsolete: true

    
    

    
  


  

        Attachment #8338295 -
        Attachment is obsolete: true

        Attachment #8339803 -
        Flags: review?(vchang)

        Attachment #8339803 -
        Attachment description: 3002. Support WPA-EAP connection state. → 0002. Support WPA-EAP connection state.

        Attachment #8339802 -
        Flags: review?(vchang) → review+

        Attachment #8339803 -
        Flags: review?(vchang) → review+

    
    

    
  
This patch only works after certificate functionalities are completed, so set block.
Depends on: 917175
Component: DOM: Device Interfaces → Wifi
Product: Core → Firefox OS
Version: Other Branch → unspecified

    
    

    
  
The user story bug this blocks (bug 922930) has a target milestone of future, so this is not a committed feature for 1.3. Non-committed features for 1.3 should not block the release.
blocking-b2g: 1.3+ → 1.3?

    
    

    
  


  
1. Check configure parameter is valid.
2. Return certificate name to DOM.

        Attachment #8339802 -
        Attachment is obsolete: true

        Attachment #8343525 -
        Flags: review?(vchang)
blocking-b2g: 1.3? → 1.4+
Target Milestone: 1.3 Sprint 6 - 12/6 → ---

    
  
Blocks: 953237

    
    

    
  
After discussing PM, it is not a 1.4+.
blocking-b2g: 1.4+ → backlog
Whiteboard: [FT:RIL]

    
    

    
  


  
update to webIDL.

        Attachment #8343525 -
        Attachment is obsolete: true

        Attachment #8343525 -
        Flags: review?(vchang)

        Attachment #8406735 -
        Flags: review?(vchang)

        Attachment #8406735 -
        Flags: review?(mrbkap)

    
    

    
  


  
Update to webIDL.

        Attachment #8343526 -
        Attachment is obsolete: true

        Attachment #8406738 -
        Flags: review?(vchang)

        Attachment #8406738 -
        Flags: review?(mrbkap)

    
    

    
  
Comment on attachment 8406735 [details] [diff] [review]
0001. Support WPA-EAP configure parameters. V3

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

I'm not the DOM peer, but the patch looks good to me, so feedback+.

::: dom/wifi/WifiWorker.js
@@ +1074,5 @@
>      for (var n = 0; n < networkConfigurationFields.length; ++n) {
>        let fieldName = networkConfigurationFields[n];
>        if (!(fieldName in config) ||
> +          (typeof(config[fieldName]) === 'undefined' ||
> +           config[fieldName] === null) ||

I'd appreciate if you could refactor here a little bit.

        Attachment #8406735 -
        Flags: review?(vchang) → feedback+

    
    

    
  
Comment on attachment 8406738 [details] [diff] [review]
0002. Support WPA-EAP connection state. V3

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

Looks good.

        Attachment #8406738 -
        Flags: review?(vchang) → feedback+

    
    

    
  


  
Address comment 42.

        Attachment #8406735 -
        Attachment is obsolete: true

        Attachment #8406735 -
        Flags: review?(mrbkap)

        Attachment #8408117 -
        Flags: review?(mrbkap)

    
    

    
  
I had a look at the patches, but I'm wondering about something: I don't see any private_key/private_key_passwd occurrence in any of the code added. This is what I'm using to connect to EAP-TLS networks. The code claims to be supporting EAP-TLS.

Dumb question: are you sure this is working?
Flags: needinfo?(chulee)

    
    

    
  
Support for user certificate and private key is removed for now after security review, so no, TLS won't work for now.
At least we can bring up PEAP and some part of TTLS as first step.
Flags: needinfo?(chulee)

    
    

    
  
(In reply to Chuck Lee [:chucklee] from comment #46)
> Support for user certificate and private key is removed for now after
> security review, so no, TLS won't work for now.
> At least we can bring up PEAP and some part of TTLS as first step.

Fair enough, but can we just make sure we won't break any user-defined wpa_supplicant.conf connection to an EAP-TLS network ?

    
    

    
  
Technically yes, but it's like a hack to handle the exception.
I think we have to check if it's acceptable to do that.
Flags: needinfo?(vchang)
Flags: needinfo?(mrbkap)
Flags: needinfo?(hchang)
Flags: needinfo?(vchang)
Summary: B2G Wifi: No EAP TLS support for WiFi → B2G Wifi: Support EAP-PEAP and EAP-TTLS with out private key

    
    

    
  
(In reply to Chuck Lee [:chucklee] from comment #48)
> Technically yes, but it's like a hack to handle the exception.
> I think we have to check if it's acceptable to do that.

Okay, let me rephrase it: if EAP-TLS gets broken, this technically means I won't be able to dogfood.

    
    

    
  
(In reply to Chuck Lee [:chucklee] from comment #46)
> Support for user certificate and private key is removed for now after
> security review, so no, TLS won't work for now.
> At least we can bring up PEAP and some part of TTLS as first step.

Can you elaborate and give references regarding this security review ?

Also, how will be stored credentials for EAP-TTLS and EAP-PEAP, if we have no keystore secure enough for EAP-TLS ? Will those be dumped into wpa_supplicant.conf ?
Flags: needinfo?(chulee)

    
    

    
  
(In reply to Alexandre LISSY :gerard-majax from comment #50)
> (In reply to Chuck Lee [:chucklee] from comment #46)
> > Support for user certificate and private key is removed for now after
> > security review, so no, TLS won't work for now.
> > At least we can bring up PEAP and some part of TTLS as first step.
> 
> Can you elaborate and give references regarding this security review ?
> 

It's done with briansmith in many discussions including IRC, email, and skype.
It was pointed out in bug 917102 comment 35.

> Also, how will be stored credentials for EAP-TTLS and EAP-PEAP, if we have
> no keystore secure enough for EAP-TLS ? Will those be dumped into
> wpa_supplicant.conf ?

wpa_supplicant.conf protect by I/O permission.
A face to face security review with pauljt also mentioned this, we agreed that storing password in that way is acceptable(for now, as it is from the first day and most android phone), but storing private key file in the almost same security level is riskier than password.
Also bug 786741 is opened for not to do so but we don't have resource for that.
Flags: needinfo?(chulee)

    
    

    
  
(In reply to Chuck Lee [:chucklee] from comment #51)
> (In reply to Alexandre LISSY :gerard-majax from comment #50)
> > (In reply to Chuck Lee [:chucklee] from comment #46)
> > > Support for user certificate and private key is removed for now after
> > > security review, so no, TLS won't work for now.
> > > At least we can bring up PEAP and some part of TTLS as first step.
> > 
> > Can you elaborate and give references regarding this security review ?
> > 
> 
> It's done with briansmith in many discussions including IRC, email, and
> skype.
> It was pointed out in bug 917102 comment 35.

Thanks for this :)

> 
> > Also, how will be stored credentials for EAP-TTLS and EAP-PEAP, if we have
> > no keystore secure enough for EAP-TLS ? Will those be dumped into
> > wpa_supplicant.conf ?
> 
> wpa_supplicant.conf protect by I/O permission.
> A face to face security review with pauljt also mentioned this, we agreed
> that storing password in that way is acceptable(for now, as it is from the
> first day and most android phone), but storing private key file in the
> almost same security level is riskier than password.

Well, that's a choice, but I'm unsure about the argument itself: why would a login/password (TTLS/PEAP) would be considered less sensitive than a certificate/passphrase ?

> Also bug 786741 is opened for not to do so but we don't have resource for
> that.

Yes, I remember about this one.

    
    

    
  
Comment on attachment 8408117 [details] [diff] [review]
0001. Support WPA-EAP configure parameters. V4

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

r=me with my comments addressed or answered.

::: dom/wifi/WifiWorker.js
@@ +1084,5 @@
> +    function hasValidProperty(name) {
> +      return ((name in config) &&
> +               typeof(config[name]) !== 'undefined' &&
> +               config[name] !== null &&
> +               config[fieldName] !== '*' &&

Using 'fieldName' on the last line looks like a bug to me.

@@ +1765,5 @@
>        pub.known = true;
>      if (net.scan_ssid === 1)
>        pub.hidden = true;
> +    if ("ca_cert" in net && net.ca_cert &&
> +        net.ca_cert.indexOf("keystore://WIFI_SERVERCERT_" === 0))

Nit: Please put braces around the 'then' part of this if statement (due to the multi-line condition).

@@ +1831,5 @@
>      }
>  
> +    function hasValidProperty(name) {
> +      return ((name in net) &&
> +              (typeof(net[name]) !== 'undefined') && (net[name] !== null));

This second line could be written as |net[name] != null| because |null == undefined|. If that's too clever for your taste, feel free to leave it as it is.

@@ +1844,5 @@
> +        net.phase1 = quote(net.phase1);
> +
> +      if (hasValidProperty("phase2")) {
> +        if (net.eap === "PEAP") {
> +          net.phase2 = quote("auth=" + net.phase2);

Should we be validating the contents of net.phase2 somehow before sticking it into our config?

        Attachment #8408117 -
        Flags: review?(mrbkap) → review+

    
    

    
  
Comment on attachment 8406738 [details] [diff] [review]
0002. Support WPA-EAP connection state. V3

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

Optimistically marking r=me based on vchang's review. Please address my question before landing, though.

::: dom/wifi/WifiWorker.js
@@ +679,5 @@
> +      return true;
> +    }
> +    if (event.indexOf("CTRL-EVENT-EAP-TLS-CERT-ERROR") !== -1) {
> +      // Cert Error
> +      WifiManager.disconnect(function(ok) {});

IIRC, disconnect will cause us to disconnect from the network and not try again until we issue an associate or reassociate command. Is that really what's intended here?

        Attachment #8406738 -
        Flags: review?(mrbkap) → review+

    
    

    
  
(In reply to Chuck Lee [:chucklee] from comment #48)
> Technically yes, but it's like a hack to handle the exception.
> I think we have to check if it's acceptable to do that.

IMO it's fine to add extra code to allow for user-modified wpa_supplicant configurations (especially for things like this where the UI doesn't allow it).
Flags: needinfo?(mrbkap)

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/6068e0c1fa67
https://hg.mozilla.org/mozilla-central/rev/dde446669ba6
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S1 (9may)

    
    

    
  
#775499 is NOT a duplicate of this one!

That other bug asks for a specific security check to get included in UI and woa_supplicant.conf: the "subject_match" which allows the user to specify *which server exactly* to expect during EAP authentication.

This bug here speaks about client certificates, which has nothing to do with server cert name validation. That's a different thing. Servetr cert names should be checked with all EAP types: PEAP, TTLS, TLS - regardless if client certificates are involved! 

No comment in this bug here touches the subject_match parameter. Could someone with a clue review this please?

    
    

    
  
This bug is enabling choosing server certificate for a WPA-EAP network, which is the summary indicates in bug 775499. Also, what the patches did are the same in these two bugs.
Since we enables support for server certificate in wifi now(bug 917102, bug 917176, and bug 917175), I think using server certificate is easier and safer than using subject_match.

    
    

    
  
This is not true.

The three bugs you mentioned are about CA certificates, NOT server certificates. What makes a WPA-EAP network secure is that the *tuple*: ( CA + *name of expected server* ) during connection attempt match user configuration.

Your three referenced bugs only enable the configuration of the first half of that tuple - the CA trust root. The server name is the second component to this, only this locks down the connection attempt to exactly your organisation's authentication server.

Bug 775499 was all the time about this second part of the tuple.

Please re-open that bug.

    
  
feature-b2g: --- → 2.0
Flags: needinfo?(hchang)
Summary: B2G Wifi: Support EAP-PEAP and EAP-TTLS with out private key → B2G Wifi: Support EAP-PEAP and EAP-TTLS without private key
blocking-b2g: backlog → ---

          tracking-b2g:
          --- → backlog

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: