917102 - [Wifi] Support import CA into NSS

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Wifi, defect)

Description

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

The API should look like
nsIDOMDOMRequest importCa(in nsIFile certFile, in DOMString certPassword, in DOMString certNickname);

Should try to add some mark to wifi-imported CA so it can be distinguished from other CAs.

Comment 1

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

Attachment: 0001. IDL Change.

Add new API in Wifi DOM for import CA.

Comment 2

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

Attachment: 0002. Add CA management backend.

Import function provided by NSS interface is not quite fit for Wifi usage: It's deeply bind with XUL for getting passwords, and we can't assign nickname for imported CA.

Although there's Bug 875188 trying to provide a solution, I think it still can't meet wifi's request on assigning nickname.

So I implement import function, mainly to provide ability to assign nickname of certification, also prevent generating dialogs asking for password in gecko.

Comment 3

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

Attachment: 0003. Add Wifi certificate service.

Add a service providing wifi CA management.

Comment 4

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

Attachment: 0004. DOM API implementation.

Implement import CA API.

Comment 5

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 818838 [details] [diff] [review]
0004. DOM API implementation.

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

Looks good for me.

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Hi, I want to understand better the purpose of this bug. Is this a good summary of the bug: "Cannot add trust to untrusted CA used in WPA Enterprise Wifi authentication"?

I am concerned that this patch seems to make the untrusted CA trusted for any website that B2G would use, even in the browser. Also, it would make the untrusted CA trusted for *any* wifi network, right? Usually, phones will allow you to mark the certificate trusted only for the one wifi network that is using it. I know that this is how it works on Windows, for example. That is usually better, because usually every different WPA enterprise network is best off with a different trusted CA.

I think it is worth having a conversation about how to do this. Besides :chucklee, who else should I invite to the conversation?

Comment 7

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

(In reply to Brian Smith (:briansmith, was :[e-mail]) from comment #6)
> Hi, I want to understand better the purpose of this bug. Is this a good
> summary of the bug: "Cannot add trust to untrusted CA used in WPA Enterprise
> Wifi authentication"?

I am afraid not.
My main purpose is import CA(especially PKCS12 files) with ability to set nickname in certain format, and skip all the xul dialogs.

> I am concerned that this patch seems to make the untrusted CA trusted for
> any website that B2G would use, even in the browser.

I tried to divide these two by |CERT_OpenCertDBFilename| but found out it was deprecated.
Is there other way to do this?

All I need is read cert from db by |CERT_FindCertByNickname()|, so if it can also get if untrusted CA, then the problem here is solved, right?
I'll test if it's works.

> Also, it would make the
> untrusted CA trusted for *any* wifi network, right? Usually, phones will
> allow you to mark the certificate trusted only for the one wifi network that
> is using it. I know that this is how it works on Windows, for example. That
> is usually better, because usually every different WPA enterprise network is
> best off with a different trusted CA.

It works in the opposite way in Firefox OS: User have to select which CA is used to authenticate the WPA network when setting up the connectiong.
And no CA will be used if user doesn't select one.
So there will be only one CA for each WAP network.

> 
> I think it is worth having a conversation about how to do this. Besides
> :chucklee, who else should I invite to the conversation?

I think we should have :vchang and :mrbkap as well.

Comment 8

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

Comment on attachment 818835 [details] [diff] [review]
0002. Add CA management backend.

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

::: dom/wifi/NssUtils.cpp
@@ +288,5 @@
> +    certTrust.sslFlags = CERTDB_VALID_CA;
> +    certTrust.emailFlags = 0;
> +    certTrust.objectSigningFlags = 0;
> +  }
> +

Applied a test mentioned in comment 7, setting all trust flags to 0.
I can still get cert by |CERT_FindCertByNickname()|.

Is settings these flags to 0 implies untrusted CA in NSS and won't affect the browser?

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

For the record, I'm going to hold off on reviewing here until we talk about this (as mentioned in comment 6 and comment 7).

Comment 10

[Ken Chang[:kenkai]]

(In reply to Blake Kaplan (:mrbkap) from comment #9)
> For the record, I'm going to hold off on reviewing here until we talk about
> this (as mentioned in comment 6 and comment 7).
Will you discuss this in IRC or vidyo?

Comment 11

[Vincent Chang[:vchang][changyihsin]]

One question here ? NssUtils::ImportPkcs12File() and NssUtils::ImportDerFile() are blocking call right ? If yes, we should not call blocking functions in WifiWorker.

Comment 12

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

(In reply to Blake Kaplan (:mrbkap) from comment #9)
> For the record, I'm going to hold off on reviewing here until we talk about
> this (as mentioned in comment 6 and comment 7).

Short summary about our meeting:

1. The code flow for connecting to an AP
   We have a UX document at https://bug926341.bugzilla.mozilla.org/attachment.cgi?id=823805
   (1) User import new CA through |WifiManager.importCa(FILE, PASSWORD, NICKNAME)|
   (2) Gaia get list of imported CAs through |WifiManager.getCaList()|
       The returned data format would look like
       {
         "serverCa" : ["NICKNAME_OF_SERVER_CA_1", "NICKNAME_OF_SERVER_CA_2", ...],
         "userCa" : ["NICKNAME_OF_USER_CA_1", "NICKNAME_OF_USER_CA_2", ...]
       }
   (3) User connect to network with CA through |WifiManager.associate({
         ssid: "SSID_OF_NETWORK",
         serverCa: "NICKNAME_OF_SERVER_CA",
         ...
       });

2. The API is to provide a wifi-sepecific way to import CAs, because we have to assign nickname for CAs, especially CAs packed in pkcs12 format in step (1).
   The nickname will be used as CA identifier in step (2) and step (3)

3. Imported CAs won't be visible to browser, I have cleared the trust of imported CA in step (1).

4. Imported CAs won't be available for all networks, user have to select one CA for each network for authentication in step (3).
   If user doesn't select one, then no CA is used.


Hope information above provides answers to some security concerns. :)

Comment 13

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Postpone target milestone to sprint5 as we most likely won't have it land within sprint4.
Let's have a checkpoint "Nov15" to see if we get the patch review+ at that moment.
If not, we might need to figure out a plan not to block bug 922930.

Comment 14

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

Attachment: 0001. IDL Change. V2

Rebase.

Comment 15

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

Attachment: 0002. Add CA management backend. V2

1. Rebase.
2. Remove trust of imported cert.
3. Check NSS initialized as brainsmith suggested on IRC.

Comment 16

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

Attachment: 0003. Add Wifi certificate service. V2

Rebase.

Comment 17

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

Attachment: 0004. DOM API implementation. V2

Rebase.

Comment 18

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

Attachment: WIP - Prototype for CA list and delete.

For test and concept verification.

Comment 19

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

Attachment: PKCS#12 Test file

Test file for import PKCS#12 file.
Must be pushed into /mnt/sdcard/chucklee_12345678.pfx

Comment 20

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

Attachment: Mozillar Root Certificate

Mozilla Root CA for testing import CRT file.
Must be pushed into /mnt/sdcard/mozilla-root.crt

Comment 21

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

Attachment: Gaia - Test app for WIFI CA API.

The test app is inside part of "UI tests" app, UI tests -> CA Tests.
Tests are hard coded, and the results are dumped to adb.

For .pfx file test, require chucklee_12345678.pfx being placed to /mnt/sdcard/
For .crt fild test, require mozilla-root.crt being placed to /mnt/sdcard/

For devices with keystore_cli app, use following commands to verify imported CAs.
mozilla root CA : adb shell keystore_cli g WIFI_CACERT_mozilla
chucklee server CA : adb shell keystore_cli g WIFI_CACERT_chucklee
chucklee user CA : adb shell keystore_cli g WIFI_USRCERT_chucklee

Comment 22

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 832081 [details] [diff] [review]
0002. Add CA management backend. V2

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

I will talk to Blake tomorrow (or ASAP) about this. I think that some of the Nickname stuff we're doing here may not work the way we expect. But, I need to understand better how the feature is supposed to work.

I believe that you can simplify the code a lot according to these review comments. But, it may be worth waiting until I've talked to Blake about things, to see if more changes might be needed.

::: dom/wifi/NssUtils.cpp
@@ +76,5 @@
> +  aItem->data[aItem->len - 2] = 0;
> +}
> +
> +char *
> +NssUtils::GetPassword(PK11SlotInfo *slot, PRBool retry, void *arg)

This should not be necessary. See below.

@@ +89,5 @@
> +{
> +  mSlot = PK11_FindSlotByName(NSS_SLOT_NAME);
> +  if (!mSlot) {
> +    mSlot = PK11_GetInternalKeySlot();
> +  }

On IRC, we agreed to just use the internal slot. This makes this code much simpler.

@@ +96,5 @@
> +    return SECFailure;
> +  }
> +
> +  SECStatus srv;
> +  if (PK11_NeedUserInit(mSlot)) {

For the internal slot, nsNSSComponent will do this.

@@ +103,5 @@
> +      return SECFailure;
> +    }
> +  }
> +
> +  PK11_SetPasswordFunc(GetPassword);

The password function is global state shared throughout Gecko. The password function is already set by nsNSSComponent when it is initialized. So, this should be removed.

@@ +104,5 @@
> +    }
> +  }
> +
> +  PK11_SetPasswordFunc(GetPassword);
> +  srv = PK11_Authenticate(mSlot, PR_TRUE, &mSlotPasswrd);

For the internal slot, nsNSSComponent will do this.

@@ +116,5 @@
> +nsresult
> +NssUtils::ImportPkcs12File(nsIFile *aFile,
> +                           const nsAString& aPassword,
> +                           const nsAString& aNickname,
> +                           uint16_t *aUsageFlag)

Please use /*out*/ to document out parameters.

@@ +120,5 @@
> +                           uint16_t *aUsageFlag)
> +{
> +  if (!aFile) {
> +    return NS_ERROR_UNEXPECTED;
> +  }

NS_ENSURE_ARG_POINTER(aFile);

@@ +123,5 @@
> +    return NS_ERROR_UNEXPECTED;
> +  }
> +  if (!mSlot) {
> +    return NS_ERROR_NOT_INITIALIZED;
> +  }

NS_ENSURE_ARG_POINTER(mSlot);
NS_ENSURE_ARG_POINTER(aUsageFlag);

@@ +125,5 @@
> +  if (!mSlot) {
> +    return NS_ERROR_NOT_INITIALIZED;
> +  }
> +
> +  nsNSSShutDownPreventionLock locker;

if (!isAlreadyShutDown()) {
  return NS_ERROR_NOT_AVAILABLE;
}

@@ +136,5 @@
> +
> +  // Create a decoder.
> +  SEC_PKCS12DecoderContext *p12dcx = NULL;
> +  p12dcx = SEC_PKCS12DecoderStart(&passwordItem, mSlot, nullptr,
> +                                  NULL, NULL, NULL, NULL, NULL);

Use MOZ_TYPE_SPECIFIC_SCOPED_POINTER_TEMPLATE to define a ScopedSEC_PKCS12DecoderContext type in ScopedNSSTypes.h, and then use that to avoid memory leaks, instead of "goto";

@@ +145,5 @@
> +
> +  // Read File
> +  uint32_t amount;
> +  char buf[FILE_BUFFER_SIZE];
> +  rv = readFile(aFile, buf, FILE_BUFFER_SIZE, &amount);

What if the file is larger than FILE_BUFFER_SIZE bytes? This will often be the case.

@@ +236,5 @@
> +    return rv;
> +  }
> +
> +  // Create certificate object.
> +  CERTCertificate *cert = CERT_DecodeCertFromPackage(buf, amount);

ScopedCERTCertificate.

@@ +244,5 @@
> +
> +  // Import certificate with nickname.
> +  *aUsageFlag |= ImportCert(cert, aNickname, false);
> +  CERT_DestroyCertificate(cert);
> +

Maybe we should just fix nsNSSCertificateDB::AddCertFromBase64 so that it actually uses its aName argument, instead of ignoring it? That would make nsNSSCertificateDB::AddCert work and you could avoid doing CERT_DecodeCertFromPackage and ImportCert directly here, since nsNSSCertificateDB::AddCert does that stuff.

@@ +260,5 @@
> +
> +  rv = NS_NewLocalFileInputStream(getter_AddRefs(fileStream), aFile);
> +  if (NS_FAILED(rv)) {
> +    return rv;
> +  }

You can use NS_ReadInputStreamToString to read a file into an nsACString in an easier way. Just remember to check that the file isn't too marge (so we don't OOM on a huge file) first.

@@ +273,5 @@
> +
> +uint16_t
> +NssUtils::ImportCert(CERTCertificate *aCert,
> +                     const nsAString& aNickname,
> +                     bool overwrite)

The overwrite argument of this function is dangerous. It it were ever true, then somebody could easily trick you into importing in a PKCS#12 file that replaced all your good certificates with their evil ones, without you even knowing. So, I suggest you remove the overwrite parameer.

@@ +283,5 @@
> +  CopyUTF16toUTF8(aNickname, userNickname);
> +  // Determine certificate nickname.
> +  if (aCert->nsCertType & NS_CERT_TYPE_SSL_SERVER) {
> +    // Server CA
> +    snprintf(nickname, sizeof(nickname), "WIFI_CACERT_%s", userNickname.get());

CACERT is the wrong type of label to use, because a cert with type NS_CERT_TYPE_SSL_SERVER will not be a CA certificate. It will be an server end-entity certificate. So, I guess "WIFI_SERVER_%s" might be a better name. But, I suspect that you don't actually need to give these certs nicknames.

In fact, I think you do not need to actually import these certificates at all. wpa_supplicant will give you the server certificate in a message. So, you probably should skip all certificates with type NS_CERT_TYPE_SSL_SERVER and only import SSL_CLIENT and NS_CERT_TYPE_SSL_CA certificates. And, you only need to give the NS_CERT_TYPE_SSL_CA certificates a nickname if they are self-signed.

Also, you need to check the return value of snprintf in case it fails. But, even better, please use nsCString and the Append() method to *safely* do this. Try to avoid using snprintf or other ANSI C string functions in Gecko. ns[AC]String is much safer because it handles error conditions like OOM in a sane way (crashing).

@@ +286,5 @@
> +    // Server CA
> +    snprintf(nickname, sizeof(nickname), "WIFI_CACERT_%s", userNickname.get());
> +    usage = WIFI_CA_USAGE_FLAG_SERVER;
> +  } else if (aCert->nsCertType & NS_CERT_TYPE_SSL_CLIENT) {
> +    // User CA

This will not be a CA certifcate. It will be a user client certificate--the kind used to authenticate the user to the access point.

@@ +294,5 @@
> +    snprintf(nickname, sizeof(nickname), "WIFI_UNKNOW_%s", userNickname.get());
> +  }
> +
> +  // Prevent overwrite existing certificate.
> +  CERTCertificate *prevCert = CERT_FindCertByNickname(mCertDb, nickname);

Use Scoped* types. In this case, ScopedCERTCertificate.

@@ +301,5 @@
> +    return usage;
> +  }
> +
> +  // Import certificate.
> +  CERT_AddTempCertToPerm(aCert, nickname, NULL);

You need to check the return value here.

Comment 23

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

Comment on attachment 832081 [details] [diff] [review]
0002. Add CA management backend. V2

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

Thanks for all the opinions!
I will apply most of them first for they shouldn't be affected on any design change.

But I think |nsNSSCertificateDB::AddCert()| is not fit the requirement here, please see the inline comments.

::: dom/wifi/NssUtils.cpp
@@ +89,5 @@
> +{
> +  mSlot = PK11_FindSlotByName(NSS_SLOT_NAME);
> +  if (!mSlot) {
> +    mSlot = PK11_GetInternalKeySlot();
> +  }

The whole block about will be removed because I can pass null pointer as slot to |SEC_PKCS12DecoderStart()|, then it will use internal slot automatically.

@@ +136,5 @@
> +
> +  // Create a decoder.
> +  SEC_PKCS12DecoderContext *p12dcx = NULL;
> +  p12dcx = SEC_PKCS12DecoderStart(&passwordItem, mSlot, nullptr,
> +                                  NULL, NULL, NULL, NULL, NULL);

Will do!

@@ +145,5 @@
> +
> +  // Read File
> +  uint32_t amount;
> +  char buf[FILE_BUFFER_SIZE];
> +  rv = readFile(aFile, buf, FILE_BUFFER_SIZE, &amount);

Use |NS_ReadInputStreamToString()| in |readFile()| instead, and set a maximum acceptable file size.

@@ +244,5 @@
> +
> +  // Import certificate with nickname.
> +  *aUsageFlag |= ImportCert(cert, aNickname, false);
> +  CERT_DestroyCertificate(cert);
> +

We need to determine a nickname prefix based on the certificate type, then use "PREFIX+nickname" as real nickname in NSS, that's what |ImportCert()| do.
But we can't do this by using |nsNSSCertificateDB::AddCert()| because we don't know the certificate type before calling it.

@@ +273,5 @@
> +
> +uint16_t
> +NssUtils::ImportCert(CERTCertificate *aCert,
> +                     const nsAString& aNickname,
> +                     bool overwrite)

I agree, overwrite will be prohibited.

@@ +283,5 @@
> +  CopyUTF16toUTF8(aNickname, userNickname);
> +  // Determine certificate nickname.
> +  if (aCert->nsCertType & NS_CERT_TYPE_SSL_SERVER) {
> +    // Server CA
> +    snprintf(nickname, sizeof(nickname), "WIFI_CACERT_%s", userNickname.get());

Thanks! NS_CERT_TYPE_SSL_CA is the flag I am looking for. I will correct it.

And I will change all ANSI C string to nsCString.

Comment 24

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

Attachment: 0001. IDL Change. V3

Rebase.

Comment 25

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

Attachment: 0002. Add CA management backend. V3

Address comment 22, including:
1. Use internal slot.
2. Use nsCstring instead of c string.
3. Use Scoped*.
4. Use NS_ENSURE_ARG_POINTER().
5. Other miner changes.

Except for |using nsNSSCertificateDB::AddCert()| for import der files due to nickname issue.

Comment 26

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

Attachment: 0003. Add Wifi certificate service. V3

Comment 27

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

Attachment: 0004. DOM API implementation. V3

Comment 28

[Blake Kaplan (:mrbkap) (inactive)]

I'm going to review this tomorrow (California time).

Comment 29

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

Attachment: 0002. Add CA management backend. V4

Change the way of handling import error.

Comment 30

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

Attachment: Settings Patch Pack

Patch pack from bug 926334, with some minor fixes.

Requires gecko with patches of following bugs, in order:
Bug 917102(CA import) -> Bug 917176(CA list) -> Bug 917175(CA delete) -> Bug 745468(Settings support) -> Bug 790056(enable WPA-EAP)

Comment 31

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8336557 [details] [diff] [review]
0001. IDL Change. V3

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

This seems reasonable to me. I'd appreciate someone who understands the certificate vocabulary a little better than me double-checking to make sure that we're using the right terms everywhere.

::: dom/wifi/nsIWifiCaService.idl
@@ +7,5 @@
> +interface nsIFile;
> +interface nsIWifiEventListener;
> +
> +[scriptable, uuid(49e8e04e-eb05-4c92-84a6-7af32310f0c3)]
> +interface nsIWifiCaService : nsISupports {

Nit: { on its own line.

Comment 32

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8338289 [details] [diff] [review]
0002. Add CA management backend. V4

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

Some questions to answer. I didn't review the NSS specific parts in detail.

::: dom/wifi/NssUtils.cpp
@@ +33,5 @@
> +{
> +  // Make sure NSS is ready.
> +  nsresult nrv;
> +  nsCOMPtr<nsISupports> dummyUsedToEnsureNSSIsInitialized
> +    = do_GetService("@mozilla.org/psm;1", &nrv);

Nit: = at the end of the previous line.

@@ +59,5 @@
> +#ifdef IS_BIG_ENDIAN
> +  memcpy(aItem.data, stringData, aItem.len);
> +#else
> +  int i;
> +  for (i = 0; i < len; i++) {

Nit: for (int i = 0;...

@@ +61,5 @@
> +#else
> +  int i;
> +  for (i = 0; i < len; i++) {
> +    aItem.data[2*i  ] = (unsigned char )(stringData[i] << 8);
> +    aItem.data[2*i+1] = (unsigned char )(stringData[i]);

Nit: No space after "char".

@@ +224,5 @@
> +  if (NS_FAILED(rv)) {
> +    return rv;
> +  }
> +
> +  rv = NS_ReadInputStreamToString(fileStream, aBuf, fileSize);

Is this on the main thread? I think we're trying pretty hard to avoid to doing I/O on the main thread, these days.

@@ +271,5 @@
> +  }
> +
> +  // Import certificate.
> +  SECStatus srv;
> +  srv = CERT_AddTempCertToPerm(aCert, nickname, NULL);

Nit: One line for this, please.

::: dom/wifi/NssUtils.h
@@ +30,5 @@
> +  NssUtils();
> +  virtual ~NssUtils();
> +
> +  // PKCS#12 Import
> +  nsresult ImportPkcs12File(nsIFile *aFile,

We decided in the mail thread to not do this, right?

@@ +45,5 @@
> +  nsCString mPkcs12FilePassword;
> +
> +  // local helper functions
> +  void nscstringToItem(const nsAString& aString,
> +                       /*out*/ SECItem& aItem);

This name is a little odd. At the very least it needs some capitals. It'd also be nice if it was a little more descriptive.

@@ +49,5 @@
> +                       /*out*/ SECItem& aItem);
> +  nsresult ImportCert(CERTCertificate *aCert,
> +                      const nsAString& aNickname,
> +                      /*out*/ uint16_t *aUsageFlag);
> +  nsresult readFile(nsIFile *aFile,

Nit: ReadFile instead of readFile.

@@ +52,5 @@
> +                      /*out*/ uint16_t *aUsageFlag);
> +  nsresult readFile(nsIFile *aFile,
> +                    /*out*/ nsCString& aBuf);
> +
> +  virtual void virtualDestroyNSSReference() {};

Nit: No ; after {}

Comment 33

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8336561 [details] [diff] [review]
0003. Add Wifi certificate service. V3

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

I think this is safe -- nsIFile is OK to be read off the main thread.

::: dom/wifi/WifiCaService.cpp
@@ +72,5 @@
> +  NS_IMETHOD Run()
> +  {
> +    MOZ_ASSERT(!NS_IsMainThread());
> +    NssUtils nssUtil;
> +    nsresult rv;

Nit: Declare this on first use:

nsresult rv = nssUtil....;

@@ +105,5 @@
> +    return NS_OK;
> +  }
> +private:
> +  int32_t mId;
> +  nsIFile *mFile;

This almost certainly needs to be a strong ref (nsCOMPtr<>).

@@ +123,5 @@
> +{
> +  MOZ_ASSERT(aListener);
> +
> +  nsresult rv;
> +  rv = NS_NewThread(getter_AddRefs(mRequestThread));

Nit: do this on one line.

Comment 34

[(no longer active)]

Comment on attachment 8336557 [details] [diff] [review]
0001. IDL Change. V3

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

I'm minusing this mostly because of the Blob issue below.  I appreciate if you could answer the questions I've asked here so that I can provide a better second round of feedback.

Thanks!

::: dom/webidl/WifiOptions.webidl
@@ +64,5 @@
> +  * WifiWorker
> +  */
> +dictionary WifiCaServiceResultOptions
> +{
> +  long            id = 0;         // opaque id.

Why are we exposing the id field to content?

@@ +65,5 @@
> +  */
> +dictionary WifiCaServiceResultOptions
> +{
> +  long            id = 0;         // opaque id.
> +  long            status = 0;     // request execution result.

What does this value mean?  What are some of the allowed values?

@@ +66,5 @@
> +dictionary WifiCaServiceResultOptions
> +{
> +  long            id = 0;         // opaque id.
> +  long            status = 0;     // request execution result.
> +  unsigned short  usageFlag = 0;  // usage flag of CA.

What does this value mean?  What are some of the allowed values?

@@ +67,5 @@
> +{
> +  long            id = 0;         // opaque id.
> +  long            status = 0;     // request execution result.
> +  unsigned short  usageFlag = 0;  // usage flag of CA.
> +  DOMString       nickname = "";  // nickname of CA.

Is this the same nickname that one passes to importCA?  If yes, I'm not sure if there is any value in returning it here again.  If the application code needs this value it can just hold on to it, if I'm not missing anything.

::: dom/wifi/nsIWifi.idl
@@ +174,5 @@
> +     *                         includes: "ServerCA" and "UserCA".
> +     *            }
> +     * onerror: We have failed to import certificate.
> +     */
> +    nsIDOMDOMRequest importCa(in DOMString certFilename,

Nit: I'm not sure why this is called "Ca" throughout the API.  This is an abbrevation, so we should use names such as "importCA", etc.

More importantly, I'm not sure why we're taking a file name argument here.  Is this a path name on the system (such as "/path/to/file.crt")?  If yes, this is a really bad thing to expose in the API.  A much better way to do this would be to accept a Blob containing the certificate directly.

::: dom/wifi/nsIWifiCaService.idl
@@ +7,5 @@
> +interface nsIFile;
> +interface nsIWifiEventListener;
> +
> +[scriptable, uuid(49e8e04e-eb05-4c92-84a6-7af32310f0c3)]
> +interface nsIWifiCaService : nsISupports {

This is an internal interface, right?  If so, then I don't think that I need to provide feedback on it.

Comment 35

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8338289 [details] [diff] [review]
0002. Add CA management backend. V4

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

Please email me or find me on IRC if you have questions.

I suggest that you try to change nsIX50CertDB::AddCert so that it does not ignore its nickname parameter. Then, you can avoid duplicating its functionality here. Let me know if you want help with this.

::: dom/wifi/NssUtils.cpp
@@ +19,5 @@
> +
> +#include "pk11func.h"
> +#include "pkcs12.h"
> +#include "secerr.h"
> +#include "certdb.h" // CERTDB_*

Please sort all of these alphabetically, as that seems to be the (new?) coding standard for Gecko.

@@ +23,5 @@
> +#include "certdb.h" // CERTDB_*
> +
> +using namespace mozilla;
> +
> +#define MAX_FILE_SIZE    4096

NIT: This is only used within one function. Please move it to that function's scope.
I suggest that you increase this limit to 16384 or so. The vast majority of certificates will be small, but some could be larger than 4K.

@@ +34,5 @@
> +  // Make sure NSS is ready.
> +  nsresult nrv;
> +  nsCOMPtr<nsISupports> dummyUsedToEnsureNSSIsInitialized
> +    = do_GetService("@mozilla.org/psm;1", &nrv);
> +  MOZ_ASSERT(NS_SUCCEEDED(nrv));

You cannot just MOZ_ASSERT these checks. You need to do this in a function that will let you properly check that things succeeded, instead of in a constructor.

@@ +37,5 @@
> +    = do_GetService("@mozilla.org/psm;1", &nrv);
> +  MOZ_ASSERT(NS_SUCCEEDED(nrv));
> +
> +  // Open DB
> +  mCertDb = CERT_GetDefaultCertDB();

You don't need to do this here. CERT_GetDefaultCertDB() is a very cheap call. You can call that function wherever you need a reference to the cert DB, instead of having the mCertDb member variable.

@@ +39,5 @@
> +
> +  // Open DB
> +  mCertDb = CERT_GetDefaultCertDB();
> +
> +  MOZ_ASSERT(srv == SECSuccess);

Do not use MOZ_ASSERT for anything that could fail at runtime.

@@ +49,5 @@
> +}
> +
> +void
> +NssUtils::nscstringToItem(const nsAString &aString,
> +                          /*out*/ SECItem &aItem)

I think that this function can be removed for now because we had agreed to remove the PKCS#12 import and the PKCS#12 import is the only function that uses this.

@@ +52,5 @@
> +NssUtils::nscstringToItem(const nsAString &aString,
> +                          /*out*/ SECItem &aItem)
> +{
> +  const PRUnichar *stringData = aString.BeginReading();
> +  int len = aString.Length();

s/int/nsAString::size_type/

@@ +53,5 @@
> +                          /*out*/ SECItem &aItem)
> +{
> +  const PRUnichar *stringData = aString.BeginReading();
> +  int len = aString.Length();
> +  SECITEM_AllocItem(aItem, static_cast<uint32_t>(sizeof(PRUnichar) * (len + 1)));

You need to check the return value here.

But, I don't think that there are any NSS functions that expect a SECItem containing a unicode string, are there?

@@ +73,5 @@
> +nsresult
> +NssUtils::ImportPkcs12File(nsIFile *aFile,
> +                           const nsAString& aPassword,
> +                           const nsAString& aNickname,
> +                           /*out*/ uint16_t *aUsageFlag)

In email, we agreed to remove the PKCS#12 import, and only support the plain certificate file import, right? I recommend that you do that, because that will accelerate the progress here a lot. Let's file a separate bug about the PKCS#12 import.

@@ +163,5 @@
> +  return NS_OK;
> +}
> +
> +nsresult
> +NssUtils::ImportDerFile(nsIFile *aFile,

See https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style

Mozilla coding style has the "*" next to to the type, not next to the variable (nsIFile* aFile). Please correct this everywhere in the patch.

@@ +192,5 @@
> +    return NS_ERROR_OUT_OF_MEMORY;
> +  }
> +
> +  // Create certificate object.
> +  ScopedCERTCertificate cert(CERT_DecodeCertFromPackage(buf, size));

I think you should just use CERT_NewTempCertificate here. Please add a comment explaining why CERT_DecodeCertFromPackage is needed if CERT_NewTempCertificate doesn't work.

@@ +195,5 @@
> +  // Create certificate object.
> +  ScopedCERTCertificate cert(CERT_DecodeCertFromPackage(buf, size));
> +  if (!cert) {
> +    return NS_ERROR_UNEXPECTED;
> +  }

try:
if (!cert) {
  return MapSECStatus(SECFailure);
}

This will give you a better error code.

@@ +246,5 @@
> +  if (aCert->nsCertType & NS_CERT_TYPE_SSL_CA) {
> +    // Server CA
> +    fullNickname = "WIFI_CACERT_";
> +    fullNickname += userNickname;
> +    *aUsageFlag |= WIFI_CA_USAGE_FLAG_SERVER;

Some (many, most) root certificates will not have NS_CERT_TYPE_SSL_CA, AFAICT.

I suggest that you use aCert->isRoot instead.

Basically, I would make the assumption that if you are importing a PKCS#12 file, then it is a user certificate, and if you are importing a plain certificate (DER) file then it is a root certificate.

@@ +247,5 @@
> +    // Server CA
> +    fullNickname = "WIFI_CACERT_";
> +    fullNickname += userNickname;
> +    *aUsageFlag |= WIFI_CA_USAGE_FLAG_SERVER;
> +  } else if (aCert->nsCertType & NS_CERT_TYPE_SSL_CLIENT) {

In the email thread, I thought we agreed we would just do the importing of the root CA certificates in this bug for now, to make sure we have things working? Importing user certificates is more complicated because it requires the PKCS#12 stuff that needs some additional work.

@@ +256,5 @@
> +  } else {
> +    return NS_ERROR_ABORT;
> +  }
> +
> +  // Prevent overwrite existing certificate.

s/overwrite/overwriting/

Comment 36

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I spoke to chucklee about this quite a bit yesterday and I completed the reviews. Please let me know what else I can help with. Thanks.

Comment 37

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

Thanks for review!

(In reply to Blake Kaplan (:mrbkap) from comment #32)
> Comment on attachment 8338289 [details] [diff] [review]
> 0002. Add CA management backend. V4
> 
> Review of attachment 8338289 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +224,5 @@
> > +  if (NS_FAILED(rv)) {
> > +    return rv;
> > +  }
> > +
> > +  rv = NS_ReadInputStreamToString(fileStream, aBuf, fileSize);
> 
> Is this on the main thread? I think we're trying pretty hard to avoid to
> doing I/O on the main thread, these days.

The caller is responsible for create a thread to run these functions.
So it will run in another thread in WifiCaService.
Blake suggests to use CryptoTask, I will figure out how to make use of it.
 
> ::: dom/wifi/NssUtils.h
> @@ +30,5 @@
> > +  NssUtils();
> > +  virtual ~NssUtils();
> > +
> > +  // PKCS#12 Import
> > +  nsresult ImportPkcs12File(nsIFile *aFile,
> 
> We decided in the mail thread to not do this, right?
> 
> @@ +45,5 @@
> > +  nsCString mPkcs12FilePassword;
> > +
> > +  // local helper functions
> > +  void nscstringToItem(const nsAString& aString,
> > +                       /*out*/ SECItem& aItem);
> 
> This name is a little odd. At the very least it needs some capitals. It'd
> also be nice if it was a little more descriptive.

I originally intend to preserve these codes for pkcs12 follow up bug, but removing these codes seems to be a better way.
So they will be removed.

Comment 38

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

Comment on attachment 8336557 [details] [diff] [review]
0001. IDL Change. V3

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

::: dom/webidl/WifiOptions.webidl
@@ +64,5 @@
> +  * WifiWorker
> +  */
> +dictionary WifiCaServiceResultOptions
> +{
> +  long            id = 0;         // opaque id.

WifiCaServiceResultOptions is meant to contain parameters passing around between javascript code and C++ code in chrome process(but different thread) as callback parameter.
So this object is not exposing to content.
Should I move it into dom/wifi/nsIWifiCaService.idl?

@@ +65,5 @@
> +  */
> +dictionary WifiCaServiceResultOptions
> +{
> +  long            id = 0;         // opaque id.
> +  long            status = 0;     // request execution result.

This is error code of the request, but it is not been defined yet.

@@ +66,5 @@
> +dictionary WifiCaServiceResultOptions
> +{
> +  long            id = 0;         // opaque id.
> +  long            status = 0;     // request execution result.
> +  unsigned short  usageFlag = 0;  // usage flag of CA.

This indicates usage of the certificate, the flag value is defined in dom/wifi/nsIWifiCaService.idl

@@ +67,5 @@
> +{
> +  long            id = 0;         // opaque id.
> +  long            status = 0;     // request execution result.
> +  unsigned short  usageFlag = 0;  // usage flag of CA.
> +  DOMString       nickname = "";  // nickname of CA.

This object is used as callback parameter, so I think it's better to contain the request information inside.
This is what nickname is used for.

::: dom/wifi/nsIWifi.idl
@@ +174,5 @@
> +     *                         includes: "ServerCA" and "UserCA".
> +     *            }
> +     * onerror: We have failed to import certificate.
> +     */
> +    nsIDOMDOMRequest importCa(in DOMString certFilename,

Because we don't have a native support for selecting certificate in device storage, we have to build an UI with devicestorage and file API.
And it's straightforward to me to take filename as argument.

I'll ask Gaia developer if sending a blob is possible.

::: dom/wifi/nsIWifiCaService.idl
@@ +7,5 @@
> +interface nsIFile;
> +interface nsIWifiEventListener;
> +
> +[scriptable, uuid(49e8e04e-eb05-4c92-84a6-7af32310f0c3)]
> +interface nsIWifiCaService : nsISupports {

Yes, and thanks for your feedbacks!

Comment 39

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

Comment on attachment 8338289 [details] [diff] [review]
0002. Add CA management backend. V4

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

::: dom/wifi/NssUtils.cpp
@@ +53,5 @@
> +                          /*out*/ SECItem &aItem)
> +{
> +  const PRUnichar *stringData = aString.BeginReading();
> +  int len = aString.Length();
> +  SECITEM_AllocItem(aItem, static_cast<uint32_t>(sizeof(PRUnichar) * (len + 1)));

If I am not misunderstood, password for pkcs12 decoder is expected in unicode.
http://mxr.mozilla.org/security/source/security/nss/lib/pkcs12/p12d.c#1146

@@ +192,5 @@
> +    return NS_ERROR_OUT_OF_MEMORY;
> +  }
> +
> +  // Create certificate object.
> +  ScopedCERTCertificate cert(CERT_DecodeCertFromPackage(buf, size));

When importing base64 encoded file(.crt, .pem, etc.), it needs to be decoded by |CERT_DecodeCertPackage()| first.
It seems |CERT_NewTempCertificate()| can't handle this to me.

@@ +195,5 @@
> +  // Create certificate object.
> +  ScopedCERTCertificate cert(CERT_DecodeCertFromPackage(buf, size));
> +  if (!cert) {
> +    return NS_ERROR_UNEXPECTED;
> +  }

Thanks a lot!

@@ +246,5 @@
> +  if (aCert->nsCertType & NS_CERT_TYPE_SSL_CA) {
> +    // Server CA
> +    fullNickname = "WIFI_CACERT_";
> +    fullNickname += userNickname;
> +    *aUsageFlag |= WIFI_CA_USAGE_FLAG_SERVER;

I have tried this function on zeroshell, it exports pkcs12 file with both root and user certificate.
I think this still need to take into consideration.

Comment 40

[(no longer active)]

Comment on attachment 8336557 [details] [diff] [review]
0001. IDL Change. V3

f=me with the suggested name change to importCA and the usage of a Blob.  If the Blob usage is not an option, please ask for another review.  Thanks!

(In reply to Chuck Lee [:chucklee] from comment #38)
> Comment on attachment 8336557 [details] [diff] [review]
> 0001. IDL Change. V3
> 
> Review of attachment 8336557 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/webidl/WifiOptions.webidl
> @@ +64,5 @@
> > +  * WifiWorker
> > +  */
> > +dictionary WifiCaServiceResultOptions
> > +{
> > +  long            id = 0;         // opaque id.
> 
> WifiCaServiceResultOptions is meant to contain parameters passing around
> between javascript code and C++ code in chrome process(but different thread)
> as callback parameter.
> So this object is not exposing to content.
> Should I move it into dom/wifi/nsIWifiCaService.idl?

I see, thanks for the explanation.  You don't need to move it, but it would probably make sense to add a comment saying what this dictionary is used for.

> @@ +65,5 @@
> > +  */
> > +dictionary WifiCaServiceResultOptions
> > +{
> > +  long            id = 0;         // opaque id.
> > +  long            status = 0;     // request execution result.
> 
> This is error code of the request, but it is not been defined yet.
> 
> @@ +66,5 @@
> > +dictionary WifiCaServiceResultOptions
> > +{
> > +  long            id = 0;         // opaque id.
> > +  long            status = 0;     // request execution result.
> > +  unsigned short  usageFlag = 0;  // usage flag of CA.
> 
> This indicates usage of the certificate, the flag value is defined in
> dom/wifi/nsIWifiCaService.idl

Both of these could use better comments so that the next person who looks at this code is not confused the same way that I was.  :-)

> @@ +67,5 @@
> > +{
> > +  long            id = 0;         // opaque id.
> > +  long            status = 0;     // request execution result.
> > +  unsigned short  usageFlag = 0;  // usage flag of CA.
> > +  DOMString       nickname = "";  // nickname of CA.
> 
> This object is used as callback parameter, so I think it's better to contain
> the request information inside.
> This is what nickname is used for.

OK.  Since this is not exposed to content, I don't care much either way.

> ::: dom/wifi/nsIWifi.idl
> @@ +174,5 @@
> > +     *                         includes: "ServerCA" and "UserCA".
> > +     *            }
> > +     * onerror: We have failed to import certificate.
> > +     */
> > +    nsIDOMDOMRequest importCa(in DOMString certFilename,
> 
> Because we don't have a native support for selecting certificate in device
> storage, we have to build an UI with devicestorage and file API.
> And it's straightforward to me to take filename as argument.
> 
> I'll ask Gaia developer if sending a blob is possible.

Please let me know if they think that it's not.  But I still believe that using a raw file path is not a good choice here if we can avoid it.

> ::: dom/wifi/nsIWifiCaService.idl
> @@ +7,5 @@
> > +interface nsIFile;
> > +interface nsIWifiEventListener;
> > +
> > +[scriptable, uuid(49e8e04e-eb05-4c92-84a6-7af32310f0c3)]
> > +interface nsIWifiCaService : nsISupports {
> 
> Yes, and thanks for your feedbacks!

You're very welcome!

Comment 41

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

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #40)
> Comment on attachment 8336557 [details] [diff] [review]
> 0001. IDL Change. V3
> 
> f=me with the suggested name change to importCA and the usage of a Blob.  If
> the Blob usage is not an option, please ask for another review.  Thanks!

I would like to change to importCert, it's would be easier to understand than importCA.

Comment 42

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

Attachment: 0001. IDL Change. V4

Address comment 31, comment 34, and comment 40, including
1. Fix nits.
2. Change API function name.
3. Use Blob instead of filename.

Comment 43

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

Attachment: 0002. Add CA management backend. V5

Address comment 32 and comment 35, including
1. Fix nits.
2. Remove codes for importing certificate in PKCS#12 format.

Comment 44

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

Attachment: 0003. Add Wifi certificate service. V4

1. Use CryptoTask instead of Runnable to do I/O outside of main thread.
2. Remove support for PKCS#12 format.

Comment 45

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

Attachment: 0004. DOM API implementation. V4

1. Changes corresponding to API name change.

Comment 46

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

Attachment: Settings Patch Pack

Update with API change.

Comment 47

[(no longer active)]

Comment on attachment 8339787 [details] [diff] [review]
0001. IDL Change. V4

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

Thanks!

::: dom/webidl/WifiOptions.webidl
@@ +67,5 @@
> +{
> +  long            id = 0;         // request id in WifiWorker.
> +  long            status = 0;     // error code of the request, 0 indicates success.
> +  unsigned short  usageFlag = 0;  // usage flag of certificate, the flag is defined
> +                                  // in dom/wifi/nsIWifiCaService.idl

Nit: mentioning the name of the IDL file should be enough...

::: dom/wifi/nsIWifi.idl
@@ +160,5 @@
>                                    in jsval info);
>  
>      /**
> +     * Import a certificate file.
> +     * @param certBlob A Blob object containing raw data of certificate to be imported.

Please document the expected format for the certificate too.

@@ +172,5 @@
> +     *                         includes: "ServerCert".
> +     *            }
> +     * onerror: We have failed to import certificate.
> +     */
> +    nsIDOMDOMRequest importCert(in nsIDOMBlob certBlob,

Hmm, sorry to make you redo this, but technically we're importing a CA certificate here, right?  So importCACert is probably an even better name.  (Please change the first line of the comment above accordingly.)

Comment 48

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

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #47)
> Comment on attachment 8339787 [details] [diff] [review]
> 0001. IDL Change. V4
> 
> Review of attachment 8339787 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks!
> 
> ::: dom/webidl/WifiOptions.webidl
> @@ +172,5 @@
> > +     *                         includes: "ServerCert".
> > +     *            }
> > +     * onerror: We have failed to import certificate.
> > +     */
> > +    nsIDOMDOMRequest importCert(in nsIDOMBlob certBlob,
> 
> Hmm, sorry to make you redo this, but technically we're importing a CA
> certificate here, right?  So importCACert is probably an even better name. 
> (Please change the first line of the comment above accordingly.)

Although it's only supports import CA certificate now, I will add support for importing user certificate in the same API.
Changing the name to importCACert will limit its usage, and need another API like importUserCert.
But there is a exception that PKCS#12 format might contain both CA and user certificate, providing two API might make Gaia confuse.
So I would like to keep the name importCert.

Other comments will be addressed, thank you!

Comment 49

[(no longer active)]

OK, sounds good.  Thanks for the due diligence!  :-)

Comment 50

[Jason Smith [:jsmith]]

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.

Comment 51

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8339788 [details] [diff] [review]
0002. Add CA management backend. V5

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

Lots of small nits, but this looks good. r=me with them picked.

::: dom/wifi/NssUtils.cpp
@@ +57,5 @@
> +  if (isAlreadyShutDown() || !isPsmReady()) {
> +    return NS_ERROR_NOT_AVAILABLE;
> +  }
> +
> +  nsresult rv;

Nit: Declare this at first use.

@@ +68,5 @@
> +  }
> +
> +  char* buf;
> +  uint32_t size;
> +  size = blobBuf.GetMutableData(&buf);

I'd combine the uint32_t and GetMutableData lines.

@@ +80,5 @@
> +    return MapSECStatus(SECFailure);
> +  }
> +
> +  // Import certificate with nickname.
> +  rv = ImportCert(cert, aNickname, aUsageFlag);

return ImportCert(...);

@@ +89,5 @@
> +nsresult
> +NssUtils::readBlob(nsIDOMBlob *aBlob,
> +                   /*out*/ nsCString& aBuf)
> +{
> +#define MAX_FILE_SIZE (16384)

Please comment as to how we came up with this number.

@@ +91,5 @@
> +                   /*out*/ nsCString& aBuf)
> +{
> +#define MAX_FILE_SIZE (16384)
> +  nsresult rv;
> +  nsCOMPtr<nsIInputStream> inputStream;

Please declare these variables at first use. This isn't C ;-)

@@ +127,5 @@
> +  CopyUTF16toUTF8(aNickname, userNickname);
> +  // Determine certificate nickname.
> +  if (aCert->isRoot) {
> +    // Server certificate
> +    fullNickname = "WIFI_SERVERCERT_";

fullNickname.AssignLiteral(...);

::: dom/wifi/NssUtils.h
@@ +28,5 @@
> +public:
> +  NssUtils();
> +  virtual ~NssUtils();
> +
> +  nsresult ImportDerBlob(nsIDOMBlob* aCertData,

Nit: Given that DER is an acronym, I'd prefer to see this written as "ImportDERBlob".

@@ +34,5 @@
> +                         /*out*/ uint16_t* aUsageFlag);
> +
> +private:
> +  // local helper functions
> +  nsresult readBlob(nsIDOMBlob *aBlob,

Nit: ReadBlob instead of readBlob.

Comment 52

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

Attachment: 0001. IDL Change. V5

Address comments in comment 47.

Comment 53

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

Attachment: 0002. Add CA management backend. V6

Address comment 51.

Comment 54

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

Attachment: 0003. Add Wifi certificate service. V5

Rebase and fix nits.

Comment 55

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

Attachment: 0004. DOM API implementation. V5

Rebase.

Comment 56

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

Attachment: Settings Patch Pack

Gaia patch updated.

Comment 57

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

Comment on attachment 8343507 [details] [diff] [review]
Settings Patch Pack

Gaia part is landed.

Comment 58

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

Attachment: 0001. IDL Change. V6

Rebase.

Comment 59

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

Attachment: 0002. Add CA management backend. V7

Rebase.

Comment 60

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

Attachment: 0003. Add Wifi certificate service. V6

Comment 61

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

Attachment: 0004. DOM API implementation. V6

Rebase

Comment 62

[Ken Chang[:kenkai]]

After discussing PM, it is not an 1.4+ bug. Put it in backlog.

Comment 63

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8371275 [details] [diff] [review]
0002. Add CA management backend. V7

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

::: dom/wifi/NssUtils.cpp
@@ +23,5 @@
> +
> +using namespace mozilla;
> +
> +static const uint16_t WIFI_CA_USAGE_FLAG_SERVER = 0x01;
> +static const uint16_t WIFI_CA_USAGE_FLAG_USER   = 0x02;

//???

@@ +71,5 @@
> +    return NS_ERROR_OUT_OF_MEMORY;
> +  }
> +
> +  // Create certificate object.
> +  ScopedCERTCertificate cert(CERT_DecodeCertFromPackage(buf, size));

Can't you just use CERT_NewTempCertificate(CERT_GetDefaultCertDB(), &bufItem, aNickname, true, true)? Note that isPerm==true should add it to the permanent database, if I understand what NSS is doing. This makes things much simpler.

@@ +81,5 @@
> +  return ImportCert(cert, aNickname, aUsageFlag);
> +}
> +
> +nsresult
> +NssUtils::ReadBlob(nsIDOMBlob *aBlob,

nit: nsIDOMBlob* aBlob,

@@ +84,5 @@
> +nsresult
> +NssUtils::ReadBlob(nsIDOMBlob *aBlob,
> +                   /*out*/ nsCString& aBuf)
> +{
> +// Suggested by briansmith.

remove this comment please.

@@ +119,5 @@
> +  nsCString userNickname, fullNickname;
> +
> +  CopyUTF16toUTF8(aNickname, userNickname);
> +  // Determine certificate nickname.
> +  if (aCert->isRoot) {

You can't use aCert->isRoot to determine whether something is a CA certificate or a user certificate, because some user certificates could be self-signed. Also, AFAICT importing the user certificates here is useless because we don't have the private key, like we discussed before. So, I still think it is better to make this API specific to importing CA certificates.

@@ +130,5 @@
> +  }
> +
> +  // Prevent overwriting existing certificate.
> +  ScopedCERTCertificate prevCert(
> +    CERT_FindCertByNickname(CERT_GetDefaultCertDB(), fullNickname.get())

Check-then-add is a racy pattern.

I believe that if you just try to add the cert with the given nickname, and it fails with a specific error code (SEC_ERROR_DUPLICATE_CERT_NAME?) then that means the cert already existed with that nickname. This avoids any races because NSS locks the database internally.

Comment 64

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8371276 [details] [diff] [review]
0003. Add Wifi certificate service. V6

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

A DOM peer should review the overall design of WifiCertService. It looks like it is trying to be an implementation of an XPCOM component, but it doesn't implement any XPCOM interface. Not sure if that is normal in DOM but I'm not used to this pattern.

::: dom/wifi/WifiCertService.cpp
@@ +30,5 @@
> +{
> +public:
> +  ImportCertTask(int32_t aId, nsIDOMBlob *aCertBlob,
> +                 const nsAString & aCertPassword,
> +                 const nsAString & aCertNickname)

NIT: Use T* x, not T *x or T * x.

@@ +52,5 @@
> +    NssUtils nssUtil;
> +
> +    // Only support DER format now.
> +    return nssUtil.ImportDERBlob(mBlob, mResult.mNickname,
> +                                        &mResult.mUsageFlag);

Consider inlining ImportDERBlob into here, since it must always be called within a CryptoTask anyway.

NIT: indention of second line.

@@ +60,5 @@
> +  {
> +    if (NS_FAILED(rv)) {
> +      mResult.mStatus = -1;
> +    }
> +    gWifiCertService->DispatchResult(mResult);

Instead of using a global here, this class should have an nsRefPtr<WifiCertService> mWifiCertService, IMO.

@@ +153,5 @@
> +                            const nsAString & aCertNickname)
> +{
> +  RefPtr<CryptoTask> task = new ImportCertTask(aId, aCertBlob, aCertPassword,
> +                                               aCertNickname);
> +  task->Dispatch("WifiImportCert");

return task->Dispatch("WifiImportCert");

Comment 65

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

Thanks for review. :)

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #63)
> @@ +119,5 @@
> > +  nsCString userNickname, fullNickname;
> > +
> > +  CopyUTF16toUTF8(aNickname, userNickname);
> > +  // Determine certificate nickname.
> > +  if (aCert->isRoot) {
> 
> You can't use aCert->isRoot to determine whether something is a CA
> certificate or a user certificate, because some user certificates could be
> self-signed. Also, AFAICT importing the user certificates here is useless
> because we don't have the private key, like we discussed before. So, I still
> think it is better to make this API specific to importing CA certificates.
> 

Even I support specific API for each type of certificate, we still need to prevent user importing wrong type of certificate.
That's what I got in Certificate Manager of Firefox Browser: I can't import a server certificate into user certificate.
It seems that it has the ability to determine type of imported certificate.
I think I would try to identifying type of |CERTCertificate| first.

> @@ +130,5 @@
> > +  }
> > +
> > +  // Prevent overwriting existing certificate.
> > +  ScopedCERTCertificate prevCert(
> > +    CERT_FindCertByNickname(CERT_GetDefaultCertDB(), fullNickname.get())
> 
> Check-then-add is a racy pattern.
> 
> I believe that if you just try to add the cert with the given nickname, and
> it fails with a specific error code (SEC_ERROR_DUPLICATE_CERT_NAME?) then
> that means the cert already existed with that nickname. This avoids any
> races because NSS locks the database internally.

I don't see any function returning this error code, I will do some test and try to find the way.

Comment 66

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

Attachment: 0002. Add Wifi certificate service. V7

Change code based on comment 63 and comment 64.
1. Remove NssUtls.cpp and move NSS manipulate  functions in NssUtil.cpp into certficate service.
2. Fix nits.
3. Add rule for identifying server certificate.
4. Certificate Service implements inner interface, like Wifi Proxy Service.

Comment 67

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

Attachment: 0003. DOM API implementation. V7

Rebase and re-order.

Comment 68

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Comment on attachment 8379611 [details] [diff] [review]
0002. Add Wifi certificate service. V7

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

The use of NSS functions looks ok, so consider that part reviewed as long as my comments are addressed (particularly the ones in ScopedNSSTypes.h). It would be cleaner to go through the nsIX509CertDB interface, but I realize that doesn't quite do what you want, so let's not hold this up for that any longer.
A DOM peer needs to review the rest of this, if they haven't already.

::: dom/wifi/WifiCertService.cpp
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

nit: modeline

@@ +4,5 @@
> +
> +#include "WifiCertService.h"
> +
> +#include "nsIPK11TokenDB.h"   // CERTCertDBHandle
> +#include "nsIPK11Token.h"     // PK11SlotInfo

I don't think either of these #includes are still necessary.

@@ +9,5 @@
> +#include "ScopedNSSTypes.h"   // ScopedCERTCertificate
> +#include "nsNetUtil.h"        // NS_ReadInputStreamToString
> +#include "nss.h"
> +#include "pkcs12.h"
> +#include "p12plcy.h"

Some of these #includes might not be necessary anymore.

@@ +20,5 @@
> +#include "nsCxPusher.h"
> +#include "nsIDOMFile.h"
> +#include "nsIWifiService.h"
> +#include "nsServiceManagerUtils.h"
> +#include "nsXULAppAPI.h" // for XRE_GetProcessType

nit: all of these #includes should be in one block, sorted alphabetically (with the WifiCertService #include at the top, separated like it is)

@@ +103,5 @@
> +  nsresult ReadBlob(/*out*/ nsCString& aBuf)
> +  {
> +    NS_ENSURE_ARG_POINTER(mBlob);
> +
> +#define MAX_FILE_SIZE (16384)

nit: if this isn't used elsewhere, I would just use the value inline. Otherwise, use a static const value instead of a #define.

@@ +162,5 @@
> +    return NS_OK;
> +  }
> +
> +  nsCOMPtr<nsIDOMBlob> mBlob;
> +  nsString mPassword;

This doesn't seem to actually be used.

::: dom/wifi/WifiCertService.h
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

nit: modeline

::: security/manager/ssl/src/ScopedNSSTypes.h
@@ +273,5 @@
>                                            SECKEY_DestroyPrivateKey)
>  MOZ_TYPE_SPECIFIC_SCOPED_POINTER_TEMPLATE(ScopedSECKEYPublicKey,
>                                            SECKEYPublicKey,
>                                            SECKEY_DestroyPublicKey)
>  

nit: no need for an empty line here

@@ +274,5 @@
>  MOZ_TYPE_SPECIFIC_SCOPED_POINTER_TEMPLATE(ScopedSECKEYPublicKey,
>                                            SECKEYPublicKey,
>                                            SECKEY_DestroyPublicKey)
>  
> +MOZ_TYPE_SPECIFIC_SCOPED_POINTER_TEMPLATE(ScopedSEC_PKCS12DecoderContext,

It doesn't look like your patch uses this, so there's no need to add it at this time.

@@ +278,5 @@
> +MOZ_TYPE_SPECIFIC_SCOPED_POINTER_TEMPLATE(ScopedSEC_PKCS12DecoderContext,
> +                                          SEC_PKCS12DecoderContext,
> +                                          SEC_PKCS12DecoderFinish)
> +
> +

nit: we only need one empty line before the closing bracket

Comment 69

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Comment on attachment 8379613 [details] [diff] [review]
0003. DOM API implementation. V7

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

There's nothing PSM-related in this patch, so if mrbkap has already r+ed this, it needs no further review.

Comment 70

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

Thanks for the fast review!
All comment will be addressed except the following one:

(In reply to David Keeler (:keeler) from comment #68)
> Comment on attachment 8379611 [details] [diff] [review]
> 0002. Add Wifi certificate service. V7
> 
> Review of attachment 8379611 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +162,5 @@
> > +    return NS_OK;
> > +  }
> > +
> > +  nsCOMPtr<nsIDOMBlob> mBlob;
> > +  nsString mPassword;
> 
> This doesn't seem to actually be used.
> 

This is for PKCS12 file, which is implemented originally but removed to fasten the review process.
I tend to preserve this for the PKCS12 follow up.

Comment 71

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

Attachment: 0002. Add Wifi certificate service. V8

Address comment 68 including:
1. Remove unnecessary include files, and reorder them.
2. Fix nits.
3. Remove ScopedSEC_PKCS12DecoderContext.

Comment 72

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

Try for current patch set : https://tbpl.mozilla.org/?tree=Try&rev=dd79fd27d1ef
bug 917102, bug 917176, and bug 917175 are tested in single run because of their dependency.

Comment 73

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Comment on attachment 8389624 [details] [diff] [review]
0002. Add Wifi certificate service. V8

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

The NSS functions look good. Again, a dom peer should review this.

::: dom/wifi/WifiCertService.cpp
@@ +8,5 @@
> +#include "mozilla/ClearOnShutdown.h"
> +#include "mozilla/ModuleUtils.h"
> +#include "mozilla/RefPtr.h"
> +#include "cert.h"
> +#include "certdb.h"           // CERTDB_*

The comment here isn't necessary.

@@ +242,5 @@
> +#undef REGET_SLOT
> +
> +NS_IMETHODIMP
> +WifiCertService::ImportCert(int32_t aId, nsIDOMBlob* aCertBlob,
> +                            const nsAString& aCertPassword,

It's still not clear to me what the password is being used for.

::: security/manager/ssl/src/ScopedNSSTypes.h
@@ +18,5 @@
>  #include "keyhi.h"
>  #include "pk11pub.h"
>  #include "sechash.h"
>  #include "secpkcs7.h"
> +#include "pkcs12.h"

nit: this doesn't need to be here anymore

Comment 74

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

(In reply to David Keeler (:keeler) from comment #73)
> Comment on attachment 8389624 [details] [diff] [review]
> 0002. Add Wifi certificate service. V8
> 
> Review of attachment 8389624 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The NSS functions look good. Again, a dom peer should review this.
> @@ +242,5 @@
> > +#undef REGET_SLOT
> > +
> > +NS_IMETHODIMP
> > +WifiCertService::ImportCert(int32_t aId, nsIDOMBlob* aCertBlob,
> > +                            const nsAString& aCertPassword,
> 
> It's still not clear to me what the password is being used for.

The API design and patches originally contains support for importing PKCS12 file, where the password is used.
It is removed with security concerns after discussion, to make the review less complex.
But it should be supported someday, the parameter is kept here for this purpose.

Comment 75

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

Comment on attachment 8389624 [details] [diff] [review]
0002. Add Wifi certificate service. V8

Hi Blake,
  Wifi Certificate Service has been through many revisions, it's better to review again. Thanks.

Comment 76

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8389624 [details] [diff] [review]
0002. Add Wifi certificate service. V8

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

::: dom/wifi/WifiCertService.h
@@ +10,5 @@
> +#include "nsCOMPtr.h"
> +#include "nsThread.h"
> +#include "mozilla/dom/WifiOptionsBinding.h"
> +
> +namespace mozilla {

I wonder if this shouldn't go in mozilla::dom, but I leave that up to your judgement.

Comment 77

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

Attachment: 0002. Add Wifi certificate service. V9

Address comment 73 and comment 76.

Comment 78

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

Attachment: 0003. DOM API implementation. V8

Rebase.

Comment 79

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

Attachment: 0001. WebIDL Change

Updated to webIDL.

Comment 80

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

Attachment: 0002. Add Wifi certificate service. V10

Update to webIDL.

Comment 81

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

Attachment: 0003. DOM API implementation. V9

Update to webIDL, add new interface.

Comment 82

[(no longer active)]

Comment on attachment 8406725 [details] [diff] [review]
0001. WebIDL Change

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

::: dom/webidl/MozWifiManager.webidl
@@ +1,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +interface Blob;

You should not need this.

@@ +90,5 @@
>  };
>  
> +[JSImplementation="@mozilla.org/mozwificertficateinfo;1",
> + Func="Navigator::HasWifiManagerSupport"]
> +interface MozWifiCertifcateInfo {

Who uses this interface and how do they access it?  Without a way to access this interface, I'm not sure if this is needed.

@@ +92,5 @@
> +[JSImplementation="@mozilla.org/mozwificertficateinfo;1",
> + Func="Navigator::HasWifiManagerSupport"]
> +interface MozWifiCertifcateInfo {
> +  [Constant] readonly attribute DOMString nickname;
> +  [Constant, Cached] readonly attribute sequence<DOMString> usage;

This is bad, but perhaps I don't need to comment on it because of the above.  If this interface is actually used by something, please include a description of what you're trying to achieve here.

::: dom/webidl/WifiOptions.webidl
@@ +59,5 @@
> +
> +
> +/**
> +  * This dictionary holds the callback parameter sent back from WifiCertService
> +  * to WifiWorker, and should only pass around in chrome process.

Nit: be passed.

::: dom/wifi/nsIWifiCertService.idl
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Note that I did not look at this file.

Comment 83

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

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #82)
> Comment on attachment 8406725 [details] [diff] [review]
> 0001. WebIDL Change
> 
> Review of attachment 8406725 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/webidl/MozWifiManager.webidl
> @@ +1,5 @@
> >  /* This Source Code Form is subject to the terms of the Mozilla Public
> >   * License, v. 2.0. If a copy of the MPL was not distributed with this
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> > +interface Blob;
> 
> You should not need this.
> 
> @@ +90,5 @@
> >  };
> >  
> > +[JSImplementation="@mozilla.org/mozwificertficateinfo;1",
> > + Func="Navigator::HasWifiManagerSupport"]
> > +interface MozWifiCertifcateInfo {
> 
> Who uses this interface and how do they access it?  Without a way to access
> this interface, I'm not sure if this is needed.
> 

It's the data structure of argument of DOMRequest.onsuccess().
It still works without these interface definitions, but I am not sure if I need to define these interfaces.

Comment 84

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

Attachment: 0001. WebIDL Change. V2

Address comment 82.

Comment 85

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

Attachment: 0003. DOM API implementation. V10

Update with WebIDL change.

Comment 86

[(no longer active)]

Comment on attachment 8408099 [details] [diff] [review]
0001. WebIDL Change. V2

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

r=me on the .webidl files.

Comment 87

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8408099 [details] [diff] [review]
0001. WebIDL Change. V2

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

::: dom/webidl/MozWifiManager.webidl
@@ +206,5 @@
> +   *                     Nickname must be unique, it causes error on redundant nickname.
> +   * onsuccess: We have successfully imported certificate. request.value is an
> +   *            object containing information of imported CA:
> +   *            {
> +   *              nickname:  Nickname of imported CA, String.

Actually, in retrospect, this won't work now that we're using webidl (or if it does work, then it should stop working in the future). We should be using a (non-constructable) interface instead. That'll also address the comment that I'm going to add on the second patch about __exposedProps__ and exposing objects to content.

Comment 88

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8408102 [details] [diff] [review]
0003. DOM API implementation. V10

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

::: dom/wifi/DOMWifiManager.js
@@ +254,5 @@
>          Services.DOMRequest.fireError(request, msg.data);
>          break;
>  
> +      case "WifiManager:importCert:Return:OK":
> +        Services.DOMRequest.fireSuccess(request, exposeReadOnly(msg.data));

Now that we're on webidl, we need to be returning a DOM object. __exposedProps__ is going away. We should be using real DOM objects here.

Comment 89

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8408102 [details] [diff] [review]
0003. DOM API implementation. V10

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

::: dom/wifi/DOMWifiManager.js
@@ +21,5 @@
>  }
>  
> +// For smaller, read-only APIs, we expose any property that doesn't begin with
> +// an underscore.
> +function exposeReadOnly(obj) {

(And then we can avoid re-adding this function.)

Comment 90

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

(In reply to Blake Kaplan (:mrbkap) from comment #87)
> Comment on attachment 8408099 [details] [diff] [review]
> 0001. WebIDL Change. V2
> 
> Review of attachment 8408099 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/webidl/MozWifiManager.webidl
> @@ +206,5 @@
> > +   *                     Nickname must be unique, it causes error on redundant nickname.
> > +   * onsuccess: We have successfully imported certificate. request.value is an
> > +   *            object containing information of imported CA:
> > +   *            {
> > +   *              nickname:  Nickname of imported CA, String.
> 
> Actually, in retrospect, this won't work now that we're using webidl (or if
> it does work, then it should stop working in the future). We should be using
> a (non-constructable) interface instead. That'll also address the comment
> that I'm going to add on the second patch about __exposedProps__ and
> exposing objects to content.

Hi Blake, Does this means add interface as attachment 8406725 [details] [diff] [review]?

Comment 91

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

Attachment: 0001. WebIDL Change. V3

Add interface for returning result per comment 87.

Comment 92

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

Attachment: 0003. DOM API implementation. V11

Address comment 88 and comment 89.

Comment 93

[(no longer active)]

Comment on attachment 8410909 [details] [diff] [review]
0001. WebIDL Change. V3

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

::: dom/webidl/MozWifiManager.webidl
@@ +90,5 @@
>  };
>  
> +[JSImplementation="@mozilla.org/mozwificertificateinfo;1",
> + Func="Navigator::HasWifiManagerSupport"]
> +interface MozWifiCertificateInfo {

Please drop the "Moz" prefix, and make this [ChromeOnly] since it's not supposed to be exposed to content as far as I can tell.  r=me with that.  If this is exposed to content, please explain how and request review again.  Thanks!

Comment 94

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

Comment on attachment 8410909 [details] [diff] [review]
0001. WebIDL Change. V3

Yes, it will expose to content as returned result of |DOMRequest.onsuccess()|, as Blake indicates in comment 87.
I am confused now should I define an interface for that or just use |Components.utils.createObjectIn|[1] to return an object.

[1] https://developer.mozilla.org/en-US/docs/Components.utils.createObjectIn

Comment 95

[(no longer active)]

Comment on attachment 8410909 [details] [diff] [review]
0001. WebIDL Change. V3

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

OK, thanks for the explanation.  Given the fact that the rest of the interfaces in this API use the Moz prefix unfortunately, this is fine for consistency I guess.  :/

Comment 96

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

Attachment: 0002. Add Wifi certificate service. V11. r=mrbkap,keeler

Rebase, change |NS_IMPL_ISUPPORTS1()| to |NS_IMPL_ISUPPORTS()|

Comment 97

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

Attachment: 0003. DOM API implementation. V12

Rebase.

Comment 98

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

Try : https://tbpl.mozilla.org/?tree=Try&rev=6b637a858cf9

Comment 99

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

Attachment: 0001. WebIDL Change. V4. r=mrbkap,ehsan

Adding interface cause try failure, and Blake accepts using createObjectIn in bug 917176.
So remove the MozWifiCertificateInfo in webidl.

Comment 100

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

Attachment: 0003. DOM API implementation. V13. r=mrbkap

Adding interface cause try failure, and Blake accepts using createObjectIn in bug 917176.
So use createObjectIn() here to create object to return.

Comment 101

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

Try with all up-to-dated WPA-EAP function patches : https://tbpl.mozilla.org/?tree=Try&rev=b586311119d2
Wifi connection tested manually on OPEN, WEP, WPA-PSK, and WPA-EAP-PEAP(with server certificate) modes.

Comment 102

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

Try looks good, let's start check in one by one.

Comment 103

[Ryan VanderMeulen [:RyanVM]]

Is there any reason these Try runs were across all platforms? This code is B2G-only, right?

Comment 104

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/a72a8e0a9b73
https://hg.mozilla.org/integration/b2g-inbound/rev/7c032a129e93
https://hg.mozilla.org/integration/b2g-inbound/rev/c9cb944fae53

Comment 105

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

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #103)
> Is there any reason these Try runs were across all platforms? This code is
> B2G-only, right?

Yes, B2G-only.
Run try on all platforms is to make sure it doesn't break anything.

Comment 106

[Ryan VanderMeulen [:RyanVM]]

A full set of builds and tests across all platforms uses over 300 machine hours of compute time and contributes to long backlogs that affects all other developers. Please be more selective about what platforms you build and run tests on. Keep in mind that you can also build on all platforms while only running tests on a subset using the "Restrict tests to platform(s)" option on TryChooser.

Comment 107

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a72a8e0a9b73
https://hg.mozilla.org/mozilla-central/rev/7c032a129e93
https://hg.mozilla.org/mozilla-central/rev/c9cb944fae53

Comment 108

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

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #106)
> A full set of builds and tests across all platforms uses over 300 machine
> hours of compute time and contributes to long backlogs that affects all
> other developers. Please be more selective about what platforms you build
> and run tests on. Keep in mind that you can also build on all platforms
> while only running tests on a subset using the "Restrict tests to
> platform(s)" option on TryChooser.

Got that,thanks!

Comment 109

[Ryan VanderMeulen [:RyanVM]]

Thank you! :)