Closed
Bug 917102
Opened 10 years ago
Closed 9 years ago
[Wifi] Support import CA into NSS
Categories
(Firefox OS Graveyard :: Wifi, defect)
Tracking
(feature-b2g:2.0, tracking-b2g:backlog)
People
(Reporter: chucklee, Assigned: chucklee)
References
Details
(Whiteboard: [bug 917102, bug 917176, bug 917175, bug 745468, and bug 790056(in order) needed for testing this patch])
Attachments
(5 files, 43 obsolete files)
2.49 KB,
application/x-pkcs12
|
Details | |
2.21 KB,
application/x-x509-ca-cert
|
Details | |
9.75 KB,
patch
|
Details | Diff | Splinter Review | |
4.83 KB,
patch
|
Details | Diff | Splinter Review | |
9.62 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•10 years ago
|
Updated•10 years ago
|
blocking-b2g: --- → 1.3+
Assignee | ||
Comment 1•10 years ago
|
||
Add new API in Wifi DOM for import CA.
Attachment #818832 -
Flags: feedback?(mrbkap)
Assignee | ||
Comment 2•10 years ago
|
||
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.
Attachment #818835 -
Flags: feedback?(vchang)
Attachment #818835 -
Flags: feedback?(mrbkap)
Attachment #818835 -
Flags: feedback?(brian)
Assignee | ||
Updated•10 years ago
|
Attachment #818832 -
Flags: feedback?(vchang)
Assignee | ||
Comment 3•10 years ago
|
||
Add a service providing wifi CA management.
Attachment #818837 -
Flags: feedback?(vchang)
Attachment #818837 -
Flags: feedback?(mrbkap)
Assignee | ||
Updated•10 years ago
|
Attachment #818835 -
Attachment description: 0002. Add CA management backend.patch → 0002. Add CA management backend.
Assignee | ||
Comment 4•10 years ago
|
||
Implement import CA API.
Attachment #818838 -
Flags: feedback?(vchang)
Attachment #818838 -
Flags: feedback?(mrbkap)
Updated•10 years ago
|
Target Milestone: --- → 1.3 Sprint 4 - 11/8
Comment 5•10 years ago
|
||
Comment on attachment 818838 [details] [diff] [review] 0004. DOM API implementation. Review of attachment 818838 [details] [diff] [review]: ----------------------------------------------------------------- Looks good for me.
Attachment #818838 -
Flags: feedback?(vchang) → feedback+
Updated•10 years ago
|
Whiteboard: [FT:RIL]
Comment 6•10 years ago
|
||
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?
Flags: needinfo?(chulee)
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to Brian Smith (:briansmith, was :bsmith@mozilla.com) 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.
Flags: needinfo?(chulee)
Assignee | ||
Comment 8•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
(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•10 years ago
|
||
One question here ? NssUtils::ImportPkcs12File() and NssUtils::ImportDerFile() are blocking call right ? If yes, we should not call blocking functions in WifiWorker.
Assignee | ||
Comment 12•10 years ago
|
||
(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. :)
Updated•10 years ago
|
Flags: needinfo?(brian)
Updated•10 years ago
|
Target Milestone: 1.3 Sprint 4 - 11/8 → 1.3 Sprint 5 - 11/22
Comment 13•10 years ago
|
||
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.
Assignee | ||
Comment 14•10 years ago
|
||
Rebase.
Attachment #818832 -
Attachment is obsolete: true
Attachment #818832 -
Flags: feedback?(vchang)
Attachment #818832 -
Flags: feedback?(mrbkap)
Attachment #832059 -
Flags: feedback?(vchang)
Attachment #832059 -
Flags: feedback?(mrbkap)
Assignee | ||
Comment 15•10 years ago
|
||
1. Rebase. 2. Remove trust of imported cert. 3. Check NSS initialized as brainsmith suggested on IRC.
Attachment #818835 -
Attachment is obsolete: true
Attachment #818835 -
Flags: feedback?(vchang)
Attachment #818835 -
Flags: feedback?(mrbkap)
Attachment #818835 -
Flags: feedback?(brian)
Attachment #832081 -
Flags: feedback?(vchang)
Attachment #832081 -
Flags: feedback?(mrbkap)
Attachment #832081 -
Flags: feedback?(brian)
Assignee | ||
Comment 16•10 years ago
|
||
Rebase.
Attachment #818837 -
Attachment is obsolete: true
Attachment #818837 -
Flags: feedback?(vchang)
Attachment #818837 -
Flags: feedback?(mrbkap)
Attachment #832082 -
Flags: feedback?(vchang)
Attachment #832082 -
Flags: feedback?(mrbkap)
Assignee | ||
Comment 17•10 years ago
|
||
Rebase.
Attachment #818838 -
Attachment is obsolete: true
Attachment #818838 -
Flags: feedback?(mrbkap)
Attachment #832084 -
Flags: feedback?(vchang)
Attachment #832084 -
Flags: feedback?(mrbkap)
Assignee | ||
Comment 18•10 years ago
|
||
For test and concept verification.
Assignee | ||
Comment 19•10 years ago
|
||
Test file for import PKCS#12 file. Must be pushed into /mnt/sdcard/chucklee_12345678.pfx
Assignee | ||
Comment 20•10 years ago
|
||
Mozilla Root CA for testing import CRT file. Must be pushed into /mnt/sdcard/mozilla-root.crt
Assignee | ||
Comment 21•10 years ago
|
||
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•10 years ago
|
||
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.
Attachment #832081 -
Flags: feedback?(brian) → feedback-
Assignee | ||
Comment 23•10 years ago
|
||
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.
Updated•10 years ago
|
Target Milestone: 1.3 Sprint 5 - 11/22 → 1.3 Sprint 6 - 12/6
Assignee | ||
Comment 24•10 years ago
|
||
Rebase.
Attachment #832059 -
Attachment is obsolete: true
Attachment #832059 -
Flags: feedback?(vchang)
Attachment #832059 -
Flags: feedback?(mrbkap)
Attachment #8336557 -
Flags: review?(vchang)
Attachment #8336557 -
Flags: review?(mrbkap)
Assignee | ||
Comment 25•10 years ago
|
||
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.
Attachment #832081 -
Attachment is obsolete: true
Attachment #832081 -
Flags: feedback?(vchang)
Attachment #832081 -
Flags: feedback?(mrbkap)
Attachment #8336559 -
Flags: review?(vchang)
Attachment #8336559 -
Flags: review?(mrbkap)
Assignee | ||
Updated•10 years ago
|
Attachment #8336557 -
Attachment description: IDL Change. V3 → 0001. IDL Change. V3
Assignee | ||
Comment 26•10 years ago
|
||
Attachment #832082 -
Attachment is obsolete: true
Attachment #832082 -
Flags: feedback?(vchang)
Attachment #832082 -
Flags: feedback?(mrbkap)
Attachment #8336561 -
Flags: review?(vchang)
Attachment #8336561 -
Flags: review?(mrbkap)
Assignee | ||
Updated•10 years ago
|
Attachment #8336559 -
Flags: review?(brian)
Assignee | ||
Comment 27•10 years ago
|
||
Attachment #832084 -
Attachment is obsolete: true
Attachment #832085 -
Attachment is obsolete: true
Attachment #832084 -
Flags: feedback?(vchang)
Attachment #832084 -
Flags: feedback?(mrbkap)
Attachment #8336562 -
Flags: review?(vchang)
Attachment #8336562 -
Flags: review?(mrbkap)
Comment 28•10 years ago
|
||
I'm going to review this tomorrow (California time).
Assignee | ||
Comment 29•10 years ago
|
||
Change the way of handling import error.
Attachment #8336559 -
Attachment is obsolete: true
Attachment #8336559 -
Flags: review?(vchang)
Attachment #8336559 -
Flags: review?(mrbkap)
Attachment #8336559 -
Flags: review?(brian)
Attachment #8338289 -
Flags: review?(vchang)
Attachment #8338289 -
Flags: review?(mrbkap)
Attachment #8338289 -
Flags: review?(brian)
Assignee | ||
Comment 30•10 years ago
|
||
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•10 years ago
|
||
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.
Attachment #8336557 -
Flags: review?(mrbkap)
Attachment #8336557 -
Flags: review+
Attachment #8336557 -
Flags: feedback?(ehsan)
Comment 32•10 years ago
|
||
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 {}
Attachment #8338289 -
Flags: review?(mrbkap)
Comment 33•10 years ago
|
||
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.
Attachment #8336561 -
Flags: review?(mrbkap) → review+
Updated•10 years ago
|
Attachment #8336562 -
Flags: review?(mrbkap) → review+
Comment 34•10 years ago
|
||
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.
Attachment #8336557 -
Flags: feedback?(ehsan) → feedback-
Comment 35•10 years ago
|
||
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/
Attachment #8338289 -
Flags: review?(brian) → review-
Comment 36•10 years ago
|
||
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.
Flags: needinfo?(brian)
Updated•10 years ago
|
Attachment #8336557 -
Flags: review?(vchang)
Updated•10 years ago
|
Attachment #8336561 -
Flags: review?(vchang)
Updated•10 years ago
|
Attachment #8336562 -
Flags: review?(vchang)
Updated•10 years ago
|
Attachment #8338289 -
Flags: review?(vchang)
Assignee | ||
Comment 37•10 years ago
|
||
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.
Assignee | ||
Comment 38•10 years ago
|
||
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!
Attachment #8336557 -
Flags: feedback- → feedback?(ehsan)
Assignee | ||
Comment 39•10 years ago
|
||
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.
Attachment #8338289 -
Flags: feedback?(brian)
Comment 40•10 years ago
|
||
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!
Attachment #8336557 -
Flags: feedback?(ehsan) → feedback+
Assignee | ||
Comment 41•10 years ago
|
||
(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.
Assignee | ||
Comment 42•10 years ago
|
||
Address comment 31, comment 34, and comment 40, including 1. Fix nits. 2. Change API function name. 3. Use Blob instead of filename.
Attachment #8336557 -
Attachment is obsolete: true
Attachment #8339787 -
Flags: review?(ehsan)
Assignee | ||
Comment 43•10 years ago
|
||
Address comment 32 and comment 35, including 1. Fix nits. 2. Remove codes for importing certificate in PKCS#12 format.
Attachment #8338289 -
Attachment is obsolete: true
Attachment #8338289 -
Flags: feedback?(brian)
Attachment #8339788 -
Flags: review?(mrbkap)
Attachment #8339788 -
Flags: review?(brian)
Assignee | ||
Comment 44•10 years ago
|
||
1. Use CryptoTask instead of Runnable to do I/O outside of main thread. 2. Remove support for PKCS#12 format.
Attachment #8336561 -
Attachment is obsolete: true
Attachment #8339790 -
Flags: review?(brian)
Assignee | ||
Comment 45•10 years ago
|
||
1. Changes corresponding to API name change.
Attachment #8336562 -
Attachment is obsolete: true
Assignee | ||
Comment 46•10 years ago
|
||
Update with API change.
Attachment #832092 -
Attachment is obsolete: true
Attachment #8338379 -
Attachment is obsolete: true
Comment 47•10 years ago
|
||
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.)
Attachment #8339787 -
Flags: review?(ehsan) → review+
Assignee | ||
Comment 48•10 years ago
|
||
(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!
Flags: needinfo?(ehsan)
Comment 50•10 years ago
|
||
The user story bug this blocks (bug 922930) has a target milestone of future, so this is not a committed feature for 1.3. Non-committed features for 1.3 should not block the release.
blocking-b2g: 1.3+ → 1.3?
Comment 51•10 years ago
|
||
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.
Attachment #8339788 -
Flags: review?(mrbkap) → review+
Updated•10 years ago
|
Whiteboard: [FT:RIL] → [FT:RIL][bug 917102, bug 745468, and bug 790056 needed for testing this patch]
Assignee | ||
Updated•10 years ago
|
Whiteboard: [FT:RIL][bug 917102, bug 745468, and bug 790056 needed for testing this patch] → [FT:RIL][bug 917102, bug 917176, bug 917175, bug 745468, and bug 790056(in order) needed for testing this patch]
Assignee | ||
Comment 52•10 years ago
|
||
Address comments in comment 47.
Attachment #8339787 -
Attachment is obsolete: true
Assignee | ||
Comment 53•10 years ago
|
||
Address comment 51.
Attachment #8339788 -
Attachment is obsolete: true
Attachment #8339788 -
Flags: review?(brian)
Attachment #8343503 -
Flags: review?(brian)
Assignee | ||
Comment 54•10 years ago
|
||
Rebase and fix nits.
Attachment #8339790 -
Attachment is obsolete: true
Attachment #8339790 -
Flags: review?(brian)
Attachment #8343504 -
Flags: review?(brian)
Assignee | ||
Comment 56•10 years ago
|
||
Gaia patch updated.
Attachment #8339821 -
Attachment is obsolete: true
Assignee | ||
Comment 57•10 years ago
|
||
Comment on attachment 8343507 [details] [diff] [review] Settings Patch Pack Gaia part is landed.
Attachment #8343507 -
Attachment is obsolete: true
Updated•10 years ago
|
blocking-b2g: 1.3? → 1.4+
Target Milestone: 1.3 Sprint 6 - 12/6 → ---
Assignee | ||
Comment 59•9 years ago
|
||
Rebase.
Attachment #8343503 -
Attachment is obsolete: true
Attachment #8343503 -
Flags: review?(brian)
Attachment #8371275 -
Flags: review?(brian)
Assignee | ||
Comment 60•9 years ago
|
||
Attachment #8343504 -
Attachment is obsolete: true
Attachment #8343504 -
Flags: review?(brian)
Attachment #8371276 -
Flags: review?(brian)
Assignee | ||
Comment 61•9 years ago
|
||
Rebase
Assignee | ||
Updated•9 years ago
|
Attachment #8343505 -
Attachment is obsolete: true
Comment 62•9 years ago
|
||
After discussing PM, it is not an 1.4+ bug. Put it in backlog.
blocking-b2g: 1.4+ → backlog
Comment 63•9 years ago
|
||
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.
Attachment #8371275 -
Flags: review?(brian) → review-
Comment 64•9 years ago
|
||
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");
Attachment #8371276 -
Flags: review?(brian) → feedback+
Assignee | ||
Comment 65•9 years ago
|
||
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.
Assignee | ||
Comment 66•9 years ago
|
||
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.
Attachment #8371275 -
Attachment is obsolete: true
Attachment #8371276 -
Attachment is obsolete: true
Attachment #8379611 -
Flags: review?(brian)
Assignee | ||
Updated•9 years ago
|
Attachment #8379611 -
Attachment description: Add Wifi certificate service. V7 → 0002. Add Wifi certificate service. V7
Assignee | ||
Comment 67•9 years ago
|
||
Rebase and re-order.
Attachment #8371277 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8379613 -
Flags: review?(brian)
Updated•9 years ago
|
Attachment #8379611 -
Flags: review?(brian)
Updated•9 years ago
|
Attachment #8379613 -
Flags: review?(brian)
Assignee | ||
Updated•9 years ago
|
Attachment #8379611 -
Flags: review?(dkeeler)
Assignee | ||
Updated•9 years ago
|
Attachment #8379613 -
Flags: review?(dkeeler)
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
Attachment #8379611 -
Flags: review?(dkeeler) → feedback+
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.
Attachment #8379613 -
Flags: review?(dkeeler)
Assignee | ||
Comment 70•9 years ago
|
||
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.
Assignee | ||
Comment 71•9 years ago
|
||
Address comment 68 including: 1. Remove unnecessary include files, and reorder them. 2. Fix nits. 3. Remove ScopedSEC_PKCS12DecoderContext.
Attachment #8379611 -
Attachment is obsolete: true
Attachment #8389624 -
Flags: review?(dkeeler)
Assignee | ||
Comment 72•9 years ago
|
||
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 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
Attachment #8389624 -
Flags: review?(dkeeler) → review+
Assignee | ||
Comment 74•9 years ago
|
||
(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.
Assignee | ||
Comment 75•9 years ago
|
||
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.
Attachment #8389624 -
Flags: review?(mrbkap)
Assignee | ||
Updated•9 years ago
|
Attachment #8379613 -
Flags: review?(mrbkap)
Comment 76•9 years ago
|
||
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.
Attachment #8389624 -
Flags: review?(mrbkap) → review+
Updated•9 years ago
|
Attachment #8379613 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 77•9 years ago
|
||
Address comment 73 and comment 76.
Attachment #8389624 -
Attachment is obsolete: true
Updated•9 years ago
|
Whiteboard: [FT:RIL][bug 917102, bug 917176, bug 917175, bug 745468, and bug 790056(in order) needed for testing this patch] → [bug 917102, bug 917176, bug 917175, bug 745468, and bug 790056(in order) needed for testing this patch]
Assignee | ||
Comment 79•9 years ago
|
||
Updated to webIDL.
Attachment #8371274 -
Attachment is obsolete: true
Attachment #8406725 -
Flags: review?(mrbkap)
Attachment #8406725 -
Flags: review?(ehsan)
Assignee | ||
Comment 80•9 years ago
|
||
Update to webIDL.
Attachment #8392702 -
Attachment is obsolete: true
Assignee | ||
Comment 81•9 years ago
|
||
Update to webIDL, add new interface.
Attachment #8392704 -
Attachment is obsolete: true
Attachment #8406728 -
Flags: review?(mrbkap)
Comment 82•9 years ago
|
||
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.
Attachment #8406725 -
Flags: review?(ehsan) → review-
Assignee | ||
Comment 83•9 years ago
|
||
(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.
Assignee | ||
Comment 84•9 years ago
|
||
Address comment 82.
Attachment #8406725 -
Attachment is obsolete: true
Attachment #8406725 -
Flags: review?(mrbkap)
Attachment #8408099 -
Flags: review?(mrbkap)
Attachment #8408099 -
Flags: review?(ehsan)
Assignee | ||
Comment 85•9 years ago
|
||
Update with WebIDL change.
Attachment #8408099 -
Attachment is obsolete: true
Attachment #8408099 -
Flags: review?(mrbkap)
Attachment #8408099 -
Flags: review?(ehsan)
Attachment #8408102 -
Flags: review?(mrbkap)
Assignee | ||
Updated•9 years ago
|
Attachment #8406728 -
Attachment is obsolete: true
Attachment #8406728 -
Flags: review?(mrbkap)
Assignee | ||
Updated•9 years ago
|
Attachment #8408099 -
Attachment is obsolete: false
Attachment #8408099 -
Flags: review?(mrbkap)
Attachment #8408099 -
Flags: review?(ehsan)
Comment 86•9 years ago
|
||
Comment on attachment 8408099 [details] [diff] [review] 0001. WebIDL Change. V2 Review of attachment 8408099 [details] [diff] [review]: ----------------------------------------------------------------- r=me on the .webidl files.
Attachment #8408099 -
Flags: review?(ehsan) → review+
Updated•9 years ago
|
Attachment #8408099 -
Flags: review?(mrbkap) → review+
Comment 87•9 years ago
|
||
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.
Attachment #8408099 -
Flags: review+
Comment 88•9 years ago
|
||
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.
Attachment #8408102 -
Flags: review?(mrbkap)
Comment 89•9 years ago
|
||
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.)
Assignee | ||
Comment 90•9 years ago
|
||
(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]?
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(mrbkap)
Assignee | ||
Comment 91•9 years ago
|
||
Add interface for returning result per comment 87.
Attachment #8408099 -
Attachment is obsolete: true
Attachment #8410909 -
Flags: review?(mrbkap)
Attachment #8410909 -
Flags: review?(ehsan)
Assignee | ||
Comment 92•9 years ago
|
||
Address comment 88 and comment 89.
Attachment #8408102 -
Attachment is obsolete: true
Attachment #8410913 -
Flags: review?(mrbkap)
Comment 93•9 years ago
|
||
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!
Attachment #8410909 -
Flags: review?(ehsan) → review+
Assignee | ||
Comment 94•9 years ago
|
||
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
Attachment #8410909 -
Flags: review+ → review?(ehsan)
Comment 95•9 years ago
|
||
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. :/
Attachment #8410909 -
Flags: review?(ehsan) → review+
Assignee | ||
Comment 96•9 years ago
|
||
Rebase, change |NS_IMPL_ISUPPORTS1()| to |NS_IMPL_ISUPPORTS()|
Attachment #8406726 -
Attachment is obsolete: true
Assignee | ||
Comment 97•9 years ago
|
||
Rebase.
Attachment #8410913 -
Attachment is obsolete: true
Attachment #8410913 -
Flags: review?(mrbkap)
Attachment #8413585 -
Flags: review?(mrbkap)
Updated•9 years ago
|
Attachment #8410909 -
Flags: review?(mrbkap) → review+
Flags: needinfo?(mrbkap)
Updated•9 years ago
|
Attachment #8413585 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 98•9 years ago
|
||
Try : https://tbpl.mozilla.org/?tree=Try&rev=6b637a858cf9
Assignee | ||
Comment 99•9 years ago
|
||
Adding interface cause try failure, and Blake accepts using createObjectIn in bug 917176. So remove the MozWifiCertificateInfo in webidl.
Attachment #8410909 -
Attachment is obsolete: true
Assignee | ||
Comment 100•9 years ago
|
||
Adding interface cause try failure, and Blake accepts using createObjectIn in bug 917176. So use createObjectIn() here to create object to return.
Attachment #8413585 -
Attachment is obsolete: true
Assignee | ||
Comment 101•9 years ago
|
||
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.
Assignee | ||
Comment 102•9 years ago
|
||
Try looks good, let's start check in one by one.
Keywords: checkin-needed
Comment 103•9 years ago
|
||
Is there any reason these Try runs were across all platforms? This code is B2G-only, right?
Flags: needinfo?(chulee)
Comment 104•9 years ago
|
||
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
Keywords: checkin-needed
Assignee | ||
Comment 105•9 years ago
|
||
(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.
Flags: needinfo?(chulee)
Comment 106•9 years ago
|
||
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.
https://hg.mozilla.org/mozilla-central/rev/a72a8e0a9b73 https://hg.mozilla.org/mozilla-central/rev/7c032a129e93 https://hg.mozilla.org/mozilla-central/rev/c9cb944fae53
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S1 (9may)
Assignee | ||
Comment 108•9 years ago
|
||
(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•9 years ago
|
||
Thank you! :)
Updated•9 years ago
|
feature-b2g: --- → 2.0
Updated•8 years ago
|
blocking-b2g: backlog → ---
tracking-b2g:
--- → backlog
You need to log in
before you can comment on or make changes to this bug.
Description
•