+++ This bug was initially created as a clone of Bug #833236 +++ Bug 833236 is for WAPI-PSK mode, we still need to support WAPI certificate mode.

Because we can not import private key to keystore now, the patch will be failed when connect to the WAPI_CERT AP. If you want to test it, you can modify gecko/dom/wifi/WifiWorker.js:1729, from net.wapi_user_cert = quote("keystore://WIFI_USERCERT_" + net.wapiUserCertificate); to net.wapi_user_cert = quote("path/user.cer");

Comment on attachment 8360159 [details] [diff] [review] gecko-support-WAPI-certificate.patch I don't think I'm qualified, ask Vincent for review here.

Comment on attachment 8360157 [details] [diff] [review] gaia-support-WAPI-certificate.patch We usually send a PR for Gaia and post the link as attachment for review.

Comment on attachment 8360159 [details] [diff] [review] gecko-support-WAPI-certificate.patch Review of attachment 8360159 [details] [diff] [review]: ----------------------------------------------------------------- Chuck, can you help to review the patches ?

Try seems better: https://tbpl.mozilla.org/?tree=Try&rev=aee163b7e6c3

Sorry, post on wrong bug. /_\

Comment on attachment 8360159 [details] [diff] [review] gecko-support-WAPI-certificate.patch Review of attachment 8360159 [details] [diff] [review]: ----------------------------------------------------------------- Please change patch to eight-line-context(with argument "-U 8") [1], transform to hg style, and change subject to "Bug XXXXXX - Patch Descriptioin". [2] [1] https://developer.mozilla.org/en/docs/Mercurial_FAQ#How_can_I_diff_and_patch_files.3F [2] https://developer.mozilla.org/en/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F ::: dom/wifi/NssUtils.cpp @@ +126,4 @@ > fullNickname += userNickname; > *aUsageFlag |= WIFI_CA_USAGE_FLAG_SERVER; > } else { > + // User certificate I think we need to a rule to identify user certificate, not in else. About the user certificate, is it a must for you? And what types of user certificate do you expect can be imported? ::: dom/wifi/WifiWorker.js @@ +1004,4 @@ > "ssid", "bssid", "psk", "wep_key0", "wep_key1", "wep_key2", "wep_key3", > "wep_tx_keyidx", "priority", "key_mgmt", "scan_ssid", "disabled", > "identity", "password", "auth_alg", "phase1", "phase2", "eap", "pin", > + "pcsc", "ca_cert", "proto", "wapi_as_cert","wapi_user_cert" Is there any reason to use "wapi_as_cert" and "wapi_user_cert" instead of existing "ca_cert" and "user_cert"? Based on the source code I found on github [1], these two parameters are used to indicate certificate files to be merged into a WAPI specific format file [2], for read into memory right after [3]. I think it can be replaced by using "ca_cert" and "user_cert" while reading config [4], and use |BIO_from_keystore| [5] to read corresponding certificates in DER format from NSS directly. Then we can get rid of the merge-read process, and prevent creating template certificate file for better security. [1] https://github.com/Dopi/JetPlatform/blob/master/delta/external/wpa_supplicant/wpa_supplicant.c#L1539 [2] https://github.com/Dopi/JetPlatform/blob/master/delta/external/wpa_supplicant/wpa_supplicant.c#L1464 [3] https://github.com/Dopi/JetPlatform/blob/master/delta/external/wpa_supplicant/wapi_config.c#L388 [4] https://github.com/Dopi/JetPlatform/blob/master/delta/external/wpa_supplicant/wpa_supplicant.c#L1619 [5] https://github.com/Dopi/JetPlatform/blob/master/delta/external/wpa_supplicant/tls_openssl.c#L44 @@ +1726,5 @@ > + if (hasValidProperty("wapiAsCertificate")) > + net.wapi_as_cert = quote("keystore://WIFI_SERVERCERT_" + net.wapiAsCertificate); > + if (hasValidProperty("wapiUserCertificate")) > + net.wapi_user_cert = quote("keystore://WIFI_USERCERT_" + net.wapiUserCertificate); > + } "keystore://" is the keyword to read certificate from keystore daemon through unix socket, but the source code [1] shows wapi_*_cert doesn't support such method. Or you have patched your own supplicant to support it? [1] https://github.com/Dopi/JetPlatform/blob/master/delta/external/wpa_supplicant/wpa_supplicant.c#L1477

(In reply to Chuck Lee [:chucklee] from comment #9) > Comment on attachment 8360159 [details] [diff] [review] > gecko-support-WAPI-certificate.patch > > Review of attachment 8360159 [details] [diff] [review]: > ----------------------------------------------------------------- > > Please change patch to eight-line-context(with argument "-U 8") [1], > transform to hg style, and change subject to "Bug XXXXXX - Patch > Descriptioin". [2] > > [1] > https://developer.mozilla.org/en/docs/ > Mercurial_FAQ#How_can_I_diff_and_patch_files.3F > [2] > https://developer.mozilla.org/en/docs/ > Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check- > in_for_me.3F > OK, I will update my patch. > ::: dom/wifi/NssUtils.cpp > @@ +126,4 @@ > > fullNickname += userNickname; > > *aUsageFlag |= WIFI_CA_USAGE_FLAG_SERVER; > > } else { > > + // User certificate > > I think we need to a rule to identify user certificate, not in else. If the certificate is not root, it should be user certificate, right? > > About the user certificate, is it a must for you? Yes. > And what types of user certificate do you expect can be imported? p7. > > ::: dom/wifi/WifiWorker.js > @@ +1004,4 @@ > > "ssid", "bssid", "psk", "wep_key0", "wep_key1", "wep_key2", "wep_key3", > > "wep_tx_keyidx", "priority", "key_mgmt", "scan_ssid", "disabled", > > "identity", "password", "auth_alg", "phase1", "phase2", "eap", "pin", > > + "pcsc", "ca_cert", "proto", "wapi_as_cert","wapi_user_cert" > > Is there any reason to use "wapi_as_cert" and "wapi_user_cert" instead of > existing "ca_cert" and "user_cert"? They are defined in FUGU's revised supplicant, you can find them under |$B2G_FUGU/external/wpa_supplicant_8/|. > > Based on the source code I found on github [1], these two parameters are > used to indicate certificate files to be merged into a WAPI specific format > file [2], for read into memory right after [3]. > > I think it can be replaced by using "ca_cert" and "user_cert" while reading > config [4], and use |BIO_from_keystore| [5] to read corresponding > certificates in DER format from NSS directly. > Then we can get rid of the merge-read process, and prevent creating template > certificate file for better security. Same here, please check the codes under wpa_supplicant_8. > > [1] > https://github.com/Dopi/JetPlatform/blob/master/delta/external/ > wpa_supplicant/wpa_supplicant.c#L1539 > [2] > https://github.com/Dopi/JetPlatform/blob/master/delta/external/ > wpa_supplicant/wpa_supplicant.c#L1464 > [3] > https://github.com/Dopi/JetPlatform/blob/master/delta/external/ > wpa_supplicant/wapi_config.c#L388 > [4] > https://github.com/Dopi/JetPlatform/blob/master/delta/external/ > wpa_supplicant/wpa_supplicant.c#L1619 > [5] > https://github.com/Dopi/JetPlatform/blob/master/delta/external/ > wpa_supplicant/tls_openssl.c#L44 > > @@ +1726,5 @@ > > + if (hasValidProperty("wapiAsCertificate")) > > + net.wapi_as_cert = quote("keystore://WIFI_SERVERCERT_" + net.wapiAsCertificate); > > + if (hasValidProperty("wapiUserCertificate")) > > + net.wapi_user_cert = quote("keystore://WIFI_USERCERT_" + net.wapiUserCertificate); > > + } > > "keystore://" is the keyword to read certificate from keystore daemon > through unix socket, but the source code [1] shows wapi_*_cert doesn't > support such method. > Or you have patched your own supplicant to support it? > > [1] > https://github.com/Dopi/JetPlatform/blob/master/delta/external/ > wpa_supplicant/wpa_supplicant.c#L1477 Same here, please check the codes under wpa_supplicant_8.

(In reply to DongShengXue from comment #10) > > ::: dom/wifi/NssUtils.cpp > > @@ +126,4 @@ > > > fullNickname += userNickname; > > > *aUsageFlag |= WIFI_CA_USAGE_FLAG_SERVER; > > > } else { > > > + // User certificate > > > > I think we need to a rule to identify user certificate, not in else. > > If the certificate is not root, it should be user certificate, right? > AFAIK, The isRoot indicates the certificate is a valid root certificate, so an invalid root certificate, maybe missing some item, is also presented as |isRoot == false|. So |isRoot == false| doesn't imply it is a user certificate. > > > > About the user certificate, is it a must for you? > Yes. > > > And what types of user certificate do you expect can be imported? > p7. > Importing user certificate is not in current scope, there might be some security/privacy issue. I think we need opinion from others. > > > > ::: dom/wifi/WifiWorker.js > > @@ +1004,4 @@ > > > "ssid", "bssid", "psk", "wep_key0", "wep_key1", "wep_key2", "wep_key3", > > > "wep_tx_keyidx", "priority", "key_mgmt", "scan_ssid", "disabled", > > > "identity", "password", "auth_alg", "phase1", "phase2", "eap", "pin", > > > + "pcsc", "ca_cert", "proto", "wapi_as_cert","wapi_user_cert" > > > > Is there any reason to use "wapi_as_cert" and "wapi_user_cert" instead of > > existing "ca_cert" and "user_cert"? > > They are defined in FUGU's revised supplicant, you can find them under > |$B2G_FUGU/external/wpa_supplicant_8/|. > I can't find wapi-related code in fugu code base you mentioned, can you provide a git link?

(In reply to Chuck Lee [:chucklee] from comment #13) > (In reply to DongShengXue from comment #10) > > > ::: dom/wifi/NssUtils.cpp > > > @@ +126,4 @@ > > > > fullNickname += userNickname; > > > > *aUsageFlag |= WIFI_CA_USAGE_FLAG_SERVER; > > > > } else { > > > > + // User certificate > > > > > > I think we need to a rule to identify user certificate, not in else. > > > > If the certificate is not root, it should be user certificate, right? > > > > AFAIK, The isRoot indicates the certificate is a valid root certificate, so > an invalid root certificate, maybe missing some item, is also presented as > |isRoot == false|. > So |isRoot == false| doesn't imply it is a user certificate. > Ok, I will check the definition of the isRoot, and update the patch. > > > > > > About the user certificate, is it a must for you? > > Yes. > > > > > And what types of user certificate do you expect can be imported? > > p7. > > > > Importing user certificate is not in current scope, there might be some > security/privacy issue. > I think we need opinion from others. > Sure, I think we can refer to Bug 868373. > > > > > > ::: dom/wifi/WifiWorker.js > > > @@ +1004,4 @@ > > > > "ssid", "bssid", "psk", "wep_key0", "wep_key1", "wep_key2", "wep_key3", > > > > "wep_tx_keyidx", "priority", "key_mgmt", "scan_ssid", "disabled", > > > > "identity", "password", "auth_alg", "phase1", "phase2", "eap", "pin", > > > > + "pcsc", "ca_cert", "proto", "wapi_as_cert","wapi_user_cert" > > > > > > Is there any reason to use "wapi_as_cert" and "wapi_user_cert" instead of > > > existing "ca_cert" and "user_cert"? > > > > They are defined in FUGU's revised supplicant, you can find them under > > |$B2G_FUGU/external/wpa_supplicant_8/|. > > > > I can't find wapi-related code in fugu code base you mentioned, can you > provide a git link? git clone https://github.com/sprd-ffos/B2G.git -b sprd ./config.sh sp7710gaplus_gonk4.0_master

What is the needinfo for here? Just an FYI that this is happening or do you need a review? I can't see any issues with this, but then I am not that familiar with this code, so Brian is definitely more qualified to review here.

Comment on attachment 8361510 [details] [diff] [review] Gecko Path V2 Review of attachment 8361510 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/wifi/NssUtils.cpp @@ +127,5 @@ > *aUsageFlag |= WIFI_CA_USAGE_FLAG_SERVER; > + } else if (aCert->isUser) { > + // User certificate > + fullNickname.AssignLiteral("WIFI_USERCERT_"); > + fullNickname += userNickname; Not sure if this is a concern because I can't find the rest of this file, but do we need to do some validation (length, metacharacters etc) on aNickname here (above). I assume that this comes from content?

Hi Paul, I like to know if you have any security concerns about importing user certificates to NSS. (In reply to Paul Theriault [:pauljt] from comment #17) > Comment on attachment 8361510 [details] [diff] [review] > Gecko Path V2 > > Review of attachment 8361510 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/wifi/NssUtils.cpp > @@ +127,5 @@ > > *aUsageFlag |= WIFI_CA_USAGE_FLAG_SERVER; > > + } else if (aCert->isUser) { > > + // User certificate > > + fullNickname.AssignLiteral("WIFI_USERCERT_"); > > + fullNickname += userNickname; > > Not sure if this is a concern because I can't find the rest of this file, > but do we need to do some validation (length, metacharacters etc) on > aNickname here (above). I assume that this comes from content? The patch is based on uncheckin-ed bug 917102, and Nickname do come from content page, the meta-characters is already handled by gaia, or should we double check in gecko?

Comment on attachment 8361510 [details] [diff] [review] Gecko Path V2 Review of attachment 8361510 [details] [diff] [review]: ----------------------------------------------------------------- The code looks good to me, but I think we should use "ca_cert"/"user_cert" instead of "wapi_as_cert"/"wapi_user_cert" in central. if that can't be done, I think this patch is better go to fugu-specific branch instead of central. And the modification involving NSS has to be reviewed by brian. ::: dom/wifi/WifiWorker.js @@ +1003,5 @@ > var networkConfigurationFields = [ > "ssid", "bssid", "psk", "wep_key0", "wep_key1", "wep_key2", "wep_key3", > "wep_tx_keyidx", "priority", "key_mgmt", "scan_ssid", "disabled", > "identity", "password", "auth_alg", "phase1", "phase2", "eap", "pin", > + "pcsc", "ca_cert", "proto", "wapi_as_cert", "wapi_user_cert" I have read the source code about WAPI config, it's a great work, but I didn't find it's necessary to use "wapi_as_cert"/"wapi_user_cert" instead of "ca_cert"/"user_cert". Leveraging "ca_cert"/"user_cert" can reduce the changes in Gaia, also provide better compatibility when porting to other platform(and future releases of Firefox OS). Also, it's seems that there's no standard or well-accepted way of implementing WAPI in wpa_supplicant. If we accept this implementation into central, we might have to support various implementations in the future. This leads to a compatibility issue. So I think this patch is better go to fugu-specific branch instead of central. Also, I like to suggest to read certificates into memory directly instead of read into a temporary cert file the read back right away[1]. [1] https://github.com/Dopi/JetPlatform/blob/master/delta/external/wpa_supplicant/wpa_supplicant.c#L1619 The referenced code is identical to fugu's code.

Comment on attachment 8360962 [details] Gaia pull request Cancel the review at first as the gecko part seems not ready currently.

Comment on attachment 8361510 [details] [diff] [review] Gecko Path V2 Chuck, could you please contact Li Gong and/or Andreas Gal about this? I had a discussion with them about WAPI and they indicated to me that we should not implement WAPI stuff directly, but instead we should have the partner that is doing China-specific customizations handle this. Please ask them about the email thread "WAPI in B2G" started by Brian Smith, January 20-27.

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #21) > Comment on attachment 8361510 [details] [diff] [review] > Gecko Path V2 > > Chuck, could you please contact Li Gong and/or Andreas Gal about this? I had > a discussion with them about WAPI and they indicated to me that we should > not implement WAPI stuff directly, but instead we should have the partner > that is doing China-specific customizations handle this. Please ask them > about the email thread "WAPI in B2G" started by Brian Smith, January 20-27. IMO, we may need to discuss among TAMs first. Thank you for your information, Brian.

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #21) > Comment on attachment 8361510 [details] [diff] [review] > Gecko Path V2 > > Chuck, could you please contact Li Gong and/or Andreas Gal about this? I had > a discussion with them about WAPI and they indicated to me that we should > not implement WAPI stuff directly, but instead we should have the partner > that is doing China-specific customizations handle this. Please ask them > about the email thread "WAPI in B2G" started by Brian Smith, January 20-27. Brian, can you loop me into this mail thread? I can't find this email from my INBOX. Thank you.

Firefox OS is not being worked on