Closed Bug 917175 Opened 6 years ago Closed 6 years ago

[Wifi] Support delete imported CA in NSS

Categories

(Firefox OS Graveyard :: Wifi, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

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

RESOLVED FIXED
2.0 S1 (9may)
feature-b2g 2.0
tracking-b2g backlog

People

(Reporter: chucklee, Assigned: chucklee)

References

Details

Attachments

(3 files, 18 obsolete files)

2.44 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
6.34 KB, patch
Details | Diff | Splinter Review
2.78 KB, patch
Details | Diff | Splinter Review
The API should look like
nsIDOMDOMRequest deleteCa(in DOMString certNickname);

Should only delete CAs imported by wifi manager, so we need to find a identifying method in Bug 917102.
Blocks: 922930
blocking-b2g: --- → 1.3+
Target Milestone: --- → 1.3 Sprint 4 - 11/8
Whiteboard: [FT:RIL]
Target Milestone: 1.3 Sprint 4 - 11/8 → 1.3 Sprint 5 - 11/22
Target Milestone: 1.3 Sprint 5 - 11/22 → 1.3 Sprint 6 - 12/6
Depends on: 917176
No longer depends on: 917102
Attached patch 0001. IDL Change. (obsolete) — Splinter Review
Attachment #8336565 - Flags: review?(vchang)
Attachment #8336565 - Flags: review?(mrbkap)
Attached patch 0002. Backend. (obsolete) — Splinter Review
Attachment #8336566 - Flags: review?(vchang)
Attachment #8336566 - Flags: review?(mrbkap)
Attachment #8336566 - Flags: review?(brian)
Attached patch 0003. DOM API implementation. (obsolete) — Splinter Review
Attachment #8336567 - Flags: review?(vchang)
Attachment #8336567 - Flags: review?(mrbkap)
I'm going to review this tomorrow (California time).
Attached patch 0002. Backend. V2 (obsolete) — Splinter Review
Update to fit change in bug 917102.
Attachment #8336566 - Attachment is obsolete: true
Attachment #8336566 - Flags: review?(vchang)
Attachment #8336566 - Flags: review?(mrbkap)
Attachment #8336566 - Flags: review?(brian)
Attachment #8338292 - Flags: review?(vchang)
Attachment #8338292 - Flags: review?(mrbkap)
Attachment #8338292 - Flags: review?(brian)
Comment on attachment 8336565 [details] [diff] [review]
0001. IDL Change.

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

One non-nit question here.

::: dom/wifi/nsIWifi.idl
@@ +196,5 @@
> +     *        Nickname of imported to be deleted.
> +     * onsuccess: We have successfully deleted certificate.
> +     * onerror: We have failed to delete certificate.
> +     */
> +    nsIDOMDOMRequest deleteCa(in DOMString certNickname);

This might be a silly question: but I think bsmith told me that nicknames are not necessarily unique. Should we be passing in the cert's fingerprint instead?
Attachment #8336565 - Flags: review?(mrbkap)
Comment on attachment 8336567 [details] [diff] [review]
0003. DOM API implementation.

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

Clearing this request until my question about the API is answered.
Attachment #8336567 - Flags: review?(mrbkap)
Comment on attachment 8338292 [details] [diff] [review]
0002. Backend. V2

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

::: dom/wifi/NssUtils.cpp
@@ +296,5 @@
> +    if (!cert) {
> +      continue;
> +    }
> +
> +    srv = SEC_DeletePermCertificate(cert);

This means that we might delete two certs for one call here. Is that intended?
Attachment #8338292 - Flags: review?(mrbkap)
Comment on attachment 8338292 [details] [diff] [review]
0002. Backend. V2

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

::: dom/wifi/NssUtils.cpp
@@ +289,5 @@
> +  SECStatus srv;
> +
> +  CopyUTF16toUTF8(aNickname, userNickname);
> +  for (int i = 0; i < 2; i++) {
> +    snprintf(nickname, sizeof(nickname), "%s%s", nicknamePrefix[i], userNickname.get());

You would need to check the return value here, if you continue using snprintf.

However, I suggest that instead you sue the much-safer ns[AC]String methods that do correct bounds checking automatically (crash on memory allocation failure).

@@ +300,5 @@
> +    srv = SEC_DeletePermCertificate(cert);
> +
> +    if (srv != SECSuccess) {
> +      return NS_ERROR_UNEXPECTED;
> +    }

Please use MapSECStatus.

::: dom/wifi/WifiCaService.cpp
@@ +115,5 @@
>      NS_DispatchToMainThread(runnable);
>    }
>  };
>  
> +class DeleteCaRunnable : public nsRunnable

I suggest replacing this with CryptoTask, which handles NSS shutdown correctly.

You need to check for NSS shutdown. Also, this will do file I/O (NSS certificate database access) on the main thread.

I suggest that you try using the CryptoTask framework to avoid the main-thread I/O and also avoid having to deal with NSS shutdown. You can find some good examples of how to use CryptoTask in the Gecko source code. (e.g. toolkit/identity, IIRC.)

I suggest you try using CryptoTask for the Importing part too.
Attachment #8338292 - Flags: review?(brian) → review-
Attachment #8336565 - Flags: review?(vchang)
Attachment #8336567 - Flags: review?(vchang)
Attachment #8338292 - Flags: review?(vchang)
(In reply to Blake Kaplan (:mrbkap) from comment #6)
> Comment on attachment 8336565 [details] [diff] [review]
> 0001. IDL Change.
> 
> Review of attachment 8336565 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> One non-nit question here.
> 
> ::: dom/wifi/nsIWifi.idl
> @@ +196,5 @@
> > +     *        Nickname of imported to be deleted.
> > +     * onsuccess: We have successfully deleted certificate.
> > +     * onerror: We have failed to delete certificate.
> > +     */
> > +    nsIDOMDOMRequest deleteCa(in DOMString certNickname);
> 
> This might be a silly question: but I think bsmith told me that nicknames
> are not necessarily unique. Should we be passing in the cert's fingerprint
> instead?

Using nickname instead of hash is because it's easier for user to select from list.

And for wifi usage, we have to ensure the nickname is unique for wpa_supplicant to get.
So there's a check in |importCa()| to prevent redundant nickname.

Because we use nickname directly in Gaia settings and wpa_supplicant config, so we don't have to maintain a database for the mapping.
(In reply to Blake Kaplan (:mrbkap) from comment #8)
> Comment on attachment 8338292 [details] [diff] [review]
> 0002. Backend. V2
> 
> Review of attachment 8338292 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/wifi/NssUtils.cpp
> @@ +296,5 @@
> > +    if (!cert) {
> > +      continue;
> > +    }
> > +
> > +    srv = SEC_DeletePermCertificate(cert);
> 
> This means that we might delete two certs for one call here. Is that
> intended?

Yes, one for server certificate, one for user certificate.
In my assumption, these two certificate should only have same nickname because they are imported at once - by pkcs12 format.

Since pkcs12 is move out of current scope, next patch will only import/delete server patches.
Attached patch 0001. IDL Change. V2 (obsolete) — Splinter Review
Change API function name.
Attachment #8336565 - Attachment is obsolete: true
Attachment #8339796 - Flags: review?(mrbkap)
Attached patch 0002. Backend. V3 (obsolete) — Splinter Review
Address comment 9, including:
1. Use nsCString instead of c type string.
2. Use MapSECStatus() for returning error code.
3. Use CryptoTask instead of Runnable to do I/O outside of main thread.
Attachment #8338292 - Attachment is obsolete: true
Attachment #8339798 - Flags: review?(mrbkap)
Attachment #8339798 - Flags: review?(brian)
Attached patch 0003. DOM API implementation. V2 (obsolete) — Splinter Review
1. Changes corresponding to API function name change.
2. Answers for API is in comment 10.
Attachment #8336567 - Attachment is obsolete: true
Attachment #8339801 - Flags: review?(mrbkap)
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?
(In reply to Chuck Lee [:chucklee] from comment #10)
> Using nickname instead of hash is because it's easier for user to select
> from list.

Sure, but the user isn't the one calling the API function ;-). It isn't a ton more work for the Gaia code to keep a mapping, which would let us have the best of both worlds. That being said...

> And for wifi usage, we have to ensure the nickname is unique for
> wpa_supplicant to get.
> So there's a check in |importCa()| to prevent redundant nickname.

I must have missed that. This needs at least a comment somewhere in the IDL.

> Because we use nickname directly in Gaia settings and wpa_supplicant config,
> so we don't have to maintain a database for the mapping.

In general, I'm against implementation details dictating API design -- there hopefully will be other implementations of this API that may not be based on wpa_supplicant and which wouldn't necessarily have this limitation. That being said, having multiple certs with the same nickname seems like Bad News (TM) for the user anyway, so it seems reasonable for me to keep it in the API.
Comment on attachment 8339796 [details] [diff] [review]
0001. IDL Change. V2

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

::: dom/wifi/nsIWifiCertService.idl
@@ +41,5 @@
> +   * @param certNickname
> +   *        Certificate nickname to delete.
> +   */
> +  void deleteCert(in long id,
> +                  in DOMString certNickname);

I think you need to bump the IID here.
Attachment #8339796 - Flags: review?(mrbkap) → review+
Comment on attachment 8339798 [details] [diff] [review]
0002. Backend. V3

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

Nits only from me.

::: dom/wifi/WifiCertService.cpp
@@ +79,5 @@
> +    mResult.mStatus = 0;
> +    mResult.mUsageFlag = 0;
> +    mResult.mNickname = aCertNickname;
> +
> +    MOZ_ASSERT(NS_IsMainThread());

Uber-nit: I'd put this assertion at the top of the function.

@@ +91,5 @@
> +    MOZ_ASSERT(!NS_IsMainThread());
> +    NssUtils nssUtil;
> +
> +    nsresult rv = nssUtil.DeleteCert(mResult.mNickname);
> +    return rv;

Nit: no need for rv.
Attachment #8339798 - Flags: review?(mrbkap) → review+
Attachment #8339801 - Flags: review?(mrbkap) → review+
(In reply to Blake Kaplan (:mrbkap) from comment #17)
> Comment on attachment 8339796 [details] [diff] [review]
> 0001. IDL Change. V2
> 
> Review of attachment 8339796 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/wifi/nsIWifiCertService.idl
> @@ +41,5 @@
> > +   * @param certNickname
> > +   *        Certificate nickname to delete.
> > +   */
> > +  void deleteCert(in long id,
> > +                  in DOMString certNickname);
> 
> I think you need to bump the IID here.

id here is used to find the corresponding callback in WifiManager, it can't be removed.
Attached patch 0001. IDL Change. V3 (obsolete) — Splinter Review
Address comment 17.
Attachment #8339796 - Attachment is obsolete: true
Attached patch 0002. Backend. V4 (obsolete) — Splinter Review
Address comment 18.
Attachment #8339798 - Attachment is obsolete: true
Attachment #8339798 - Flags: review?(brian)
blocking-b2g: 1.3? → 1.4+
Target Milestone: 1.3 Sprint 6 - 12/6 → ---
Attached patch 0001. IDL Change. V4 (obsolete) — Splinter Review
Rebase.
Attachment #8343511 - Attachment is obsolete: true
Attached patch 0002. Backend. V5 (obsolete) — Splinter Review
Rebase.
Attachment #8343517 - Attachment is obsolete: true
Attachment #8343517 - Flags: review?(brian)
Attachment #8371281 - Flags: review?(brian)
After discussing PM, it is not an 1.4+ bug. Put it in backlog.
blocking-b2g: 1.4+ → backlog
Comment on attachment 8371281 [details] [diff] [review]
0002. Backend. V5

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

::: dom/wifi/NssUtils.cpp
@@ +166,5 @@
> +  ScopedCERTCertificate cert(
> +    CERT_FindCertByNickname(CERT_GetDefaultCertDB(), serverCertName.get())
> +  );
> +  if (!cert) {
> +    return NS_ERROR_UNEXPECTED;

Wouldn't MapSECStatus(SECFailure) be better here?

::: dom/wifi/WifiCertService.cpp
@@ +71,5 @@
>  
> +class DeleteCertTask MOZ_FINAL: public CryptoTask
> +{
> +public:
> +  DeleteCertTask(int32_t aId, const nsAString & aCertNickname)

NIT: const nsAString& aCertNickname

@@ +89,5 @@
> +  {
> +    MOZ_ASSERT(!NS_IsMainThread());
> +    NssUtils nssUtil;
> +
> +    return nssUtil.DeleteCert(mResult.mNickname);

Consider just inlining nssUtil.DeleteCert into here.

@@ +194,5 @@
>    return NS_OK;
>  }
>  
> +NS_IMETHODIMP
> +WifiCertService::DeleteCert(int32_t aId, const nsAString & aCertNickname)

NIT: const nsAString&

@@ +197,5 @@
> +NS_IMETHODIMP
> +WifiCertService::DeleteCert(int32_t aId, const nsAString & aCertNickname)
> +{
> +  RefPtr<CryptoTask> task = new DeleteCertTask(aId, aCertNickname);
> +  task->Dispatch("WifiDeleteCert");

NIT: return task->Dispatch("WifiDeleteCert");
Attachment #8371281 - Flags: review?(brian) → review+
Inline nssUtil.DeleteCert() and fix nits per comment 27.
Attachment #8371281 - Attachment is obsolete: true
Whiteboard: [FT:RIL]
Update to webIDL.
Attachment #8371280 - Attachment is obsolete: true
Attachment #8406733 - Flags: review?(mrbkap)
Attached patch 0003. DOM API implementation. V6 (obsolete) — Splinter Review
Update to webIDL, use new interface.
Attachment #8392707 - Attachment is obsolete: true
Attachment #8406734 - Flags: review?(mrbkap)
Attached patch 0003. DOM API implementation. V7 (obsolete) — Splinter Review
Rebase based on bug 917176.
Attachment #8406734 - Attachment is obsolete: true
Attachment #8406734 - Flags: review?(mrbkap)
Attachment #8408110 - Flags: review?(mrbkap)
Attachment #8406733 - Flags: review?(mrbkap) → review+
Attachment #8408110 - Flags: review?(mrbkap) → review+
Attachment #8379617 - Attachment description: 0002. Backend. V6 → 0002. Backend. V6. r=mrbkap,briansmith
Attachment #8406733 - Attachment description: 0001. WebIDL Change → 0001. WebIDL Change. r=mrbkap
Target Milestone: --- → 2.0 S1 (9may)
feature-b2g: --- → 2.0
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.