remove token arguments from nsIX509CertDB.importPKCS12File and exportPKCS12File

RESOLVED FIXED in Firefox 54

Status

()

Core
Security: PSM
P1
normal
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: keeler, Assigned: keeler)

Tracking

({addon-compat})

unspecified
mozilla54
addon-compat
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [psm-assigned])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

As far as I can tell, the token arguments to nsIX509CertDB.importPKCS12File and exportPKCS12File are always null (even in add-ons[0]). Furthermore, exportPKCS12File only uses the argument to make sure the user is logged in to the token that the given certificates are presumably on. However, there is no guarantee that the certificates are on that token. More to the point, if the user is not logged in to the token the certificates are on, the platform will ask the user to log in anyway, so the argument is unnecessary in that case.

[0] There is one add-on where this argument isn't null, but it appears to be using ctypes to call PK11_GetInternalKeySlot directly. It's not clear that this works.
Comment hidden (mozreview-request)

Comment 2

7 months ago
mozreview-review
Comment on attachment 8831354 [details]
bug 1334694 - remove token arguments from nsIX509CertDB.importPKCS12File and exportPKCS12File

https://reviewboard.mozilla.org/r/107912/#review109166

Ripping out nsPKCS12Blob related code always makes me happy.

::: security/manager/ssl/nsNSSCertificateDB.cpp:35
(Diff revision 1)
>  #include "nsNSSCertTrust.h"
>  #include "nsNSSCertificate.h"
>  #include "nsNSSComponent.h"
>  #include "nsNSSHelper.h"
>  #include "nsNSSShutDown.h"
>  #include "nsPK11TokenDB.h"

Nit: Looks like we can get rid of this include as well.

::: security/manager/ssl/nsPKCS12Blob.h:45
(Diff revision 1)
>    nsCOMPtr<nsIInterfaceRequestor> mUIContext;
>  
>    // local helper functions
>    nsresult getPKCS12FilePassword(SECItem *);
>    nsresult newPKCS12FilePassword(SECItem *);
>    nsresult inputToDecoder(SEC_PKCS12DecoderContext *, nsIFile *);

Looks like we should include p12.h for `SEC_PKCS12DecoderContext` and other related types in nsPKCS12Blob.cpp.

::: security/manager/ssl/nsPKCS12Blob.cpp
(Diff revision 1)
> -#include "nsIDirectoryService.h"
>  #include "nsIFile.h"
>  #include "nsIInputStream.h"
> -#include "nsKeygenHandler.h" // For GetSlotWithMechanism
>  #include "nsNSSCertificate.h"
> -#include "nsNSSComponent.h"

I believe we need to keep this because `nickname_collision()` references `NS_NSSCOMPONENT_CID`.

::: security/manager/ssl/nsPKCS12Blob.cpp
(Diff revision 1)
>  #include "nsIFile.h"
>  #include "nsIInputStream.h"
> -#include "nsKeygenHandler.h" // For GetSlotWithMechanism
>  #include "nsNSSCertificate.h"
> -#include "nsNSSComponent.h"
> -#include "nsNSSHelper.h"

Looks like this file still uses `PipUIContext`, so we should keep the include.
Attachment #8831354 - Flags: review?(cykesiopka.bmo) → review+
(Assignee)

Comment 3

7 months ago
mozreview-review-reply
Comment on attachment 8831354 [details]
bug 1334694 - remove token arguments from nsIX509CertDB.importPKCS12File and exportPKCS12File

https://reviewboard.mozilla.org/r/107912/#review109166

Thanks!

> Nit: Looks like we can get rid of this include as well.

Cool.

> Looks like we should include p12.h for `SEC_PKCS12DecoderContext` and other related types in nsPKCS12Blob.cpp.

Ok - fixed.

> I believe we need to keep this because `nickname_collision()` references `NS_NSSCOMPONENT_CID`.

Fixed.

> Looks like this file still uses `PipUIContext`, so we should keep the include.

Fixed.
Comment hidden (mozreview-request)
(Assignee)

Comment 5

7 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=02d4b78c189e

Comment 6

7 months ago
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/da9c1e0ddf0f
remove token arguments from nsIX509CertDB.importPKCS12File and exportPKCS12File r=Cykesiopka

Comment 7

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/da9c1e0ddf0f
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.