Closed Bug 1357391 Opened 7 years ago Closed 7 years ago

Allow an extension to configure Firefox security devices

Categories

(WebExtensions :: Untriaged, defect, P5)

Product:
WebExtensions
Component:
Untriaged
Type:
defect

Tracking

(firefox57 wontfix, firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: wouter, Assigned: wouter, Mentored, NeedInfo)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [design-decision-approved][triaged])

Attachments

(1 file, 1 obsolete file)

Bug 1357391 - Implement a PKCS#11 management API
59 bytes, text/x-review-board-request
zombie
: review+
kmag
: review+
ritu
: approval-mozilla-beta-
Details 
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20170401214539

Steps to reproduce:

Currently have an extension (https://addons.mozilla.org/en-US/firefox/addon/belgium-eid/) which enables our PKCS#11 module (https://eid.belgium.be/) that allows Belgian citizens to do their tax declaration (amongst other things) using their electronic ID card (a smartcard) and firefox.

The addon uses javascript with the XPCOMUtils.jsm stuff to enable the PKCS#11 module, since telling our users to go to preferences->advanced->certificates->security devices->load->(fill in data)->ok is likely to cause much confusion; if that is the only way to configure PKCS#11 modules, most people will not manage.

The new WebExtensions API does not allow to configure PKCS#11 AFAICS, so we'll need another way to get this to do what we need.

I was suggested to file a bug here on the #webextensions channel on IRC, so here you go.

In short, I'm looking for a way to enable a PKCS#11 module in firefox without having to tell the user to do it manually. If this can be done through WebExtensions, then WebExtensions it will be, but it doesn't *have* to be that, as far as I'm concerned.
Could you outline what sort of APIs or XPCOM utils you use please?
following up on other bug
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
Flags: needinfo?(w)
Resolution: --- → DUPLICATE
See Also: → 1344888
Sorry, this is not a duplicate of 1344888.

First, the other bug is about certificate management; this one is about managing PKCS#11 modules. While the two are related in that PKCS#11 modules add additional certificates to NSS, and in that if you add a PKCS#11 module to Firefox you might also want to manage the trust of certificates that the PKCS#11 module is expected to deal with, the two are not the same.

Second, as I've mentioned before, I'm not convinced that adding features to WE to deal with this is the best way forward. Currently, pretty much every PKCS#11 module has their own extension in addons.mozilla.org, where the only functionality of that extension is to add the PKCS#11 module to Firefox. This is suboptimal; pretty much the only way the extension can be implemented is to add a hardcoded list of locations where the extension cord possibly be found, and try to add each of them in suggestion until the middle can be loaded. As soon as the user chooses to install the middle in a nonstandard location, this will fail, making this a brittle way of handling modules.

Additionally, the current method requires PKCS#11 module authors, who are typically C programmers, to write an extension in Javascript, a language that they are likely to not be familiar with. While not an unsurmountable problem, it is not ideal.

Finally, it leads to a lot of code duplication, since all these extensions do almost, but not quite, the same thing.

I would like instead to point to the 13 year old bug#248722, which asks for a way to add PKCS#11 modules from outside Firefox, by having a system-wide module configuration. It would be much easier for everyone involved to have that.
Status: RESOLVED → UNCONFIRMED
Flags: needinfo?(w)
Resolution: DUPLICATE → ---
Well, since you seem to be arguing that implementing this is a bad idea, I guess invalid id a better resolution than duplicate.
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → INVALID
Sigh.

Yes, that's what I'm arguing. However, when push comes to shove, I'm not involved with any part of mozilla, and it's not my call to make. Meanwhile, there still needs to be *some* replacement functionality for the stuff that's available to legacy add-ons currently, before support for legacy add-ons gets dropped from firefox -- and that *is* something that needs proper resolution, more than an "invalid" one.

If you're telling me that having this bug open to ensure that happens isn't the right way to deal with this, I would like some guidance as to how to make sure that it does happen. Otherwise, can this bug be kept open until the required functionality has been written?

Thanks,
Status: RESOLVED → UNCONFIRMED
Resolution: INVALID → ---
That's not a valid point, there are multiple things that have no replacement. This is more than likely one of those things.
So what do you suggest I tell my users then, if they want to use Firefox to declare their taxes?

"You have to configure it manually, here is a bunch of screenshots on how to do so"? The UI to configure PKCS#11 modules in firefox is a real PITA, that just isn't an option...
Flags: needinfo?(amckay)
Dan, following on from our meeting, my main concern here is the mitigation of any security risks. This involves interaction with some hardware and the assumption is that the user has already installed that? Copying and pasting a bunch of stuff from a web page into Firefox, without knowing what it does, seems quite risky already.
Flags: needinfo?(amckay) → needinfo?(dveditz)
Hello Andy,

To clarify some things about our current extension: (https://addons.mozilla.org/en-US/firefox/addon/belgium-eid/)
All it does is checking if a library with a certain name exists in some predefined folders, and if it is found, add it to the firefox security devices.

Which is basically automating following steps for our users: (going to menu, select 'Options', Advanced, Security Devices, clicking 'load', enter the path of our pkcs11 module). 


About mitigating security risks:
-) our pkcs11 module is signed with a public trusted signing certificate (so firefox could check that before adding it into the security devices list)
-) our pkcs11 module is installed in a system folder, so it was installed with admin permissions
(In reply to Andy McKay [:andym] from comment #9)
> Dan, following on from our meeting, my main concern here is the mitigation
> of any security risks. This involves interaction with some hardware and the
> assumption is that the user has already installed that? Copying and pasting
> a bunch of stuff from a web page into Firefox, without knowing what it does,
> seems quite risky already.

A pkcs11 module doesn't necessarily need to connect to hardware, but in practice they're usually to provide support for smartcards. We've not ever done much to support them, because we had the escape valve of addons which could do the work. The built-in UX is terrible and more than you could expect a user to do. Teaching users to follow cryptic instructions on web pages likely leads to worse outcomes than "install this thing from a trusted source". The PCKS11 module itself is installed when the hardware is installed, like adding other kinds of hardware that requires drivers.

Crypto modules can completely subvert our own crypto. It's valid in certain circumstances (such as this one), but certainly abusable. I'd rather have a WebExtension API with a permission we can use to trigger reviews (and blocklist) than an "autoload from a magic location" mechanism like bug 248722 (long an abuse-point for dodgy plugins). Another advantage for Web Extensions is that users can see and uninstall WebExtension but would have no clue any autoloading was going on.

I don't know where you'd want to prioritize implementation of such an API (230K Belgian users, 13K Estonian; any others?) but it's reasonable from a security POV.
Flags: needinfo?(dveditz)
Thanks Dan. Based on comment 11, I can't see much to stop someone figuring out and implementing this API.
Priority: -- → P5
Whiteboard: [design-decision-approved][triaged]
I see you marked this as "P5". Does that mean we're at a "patches are welcome" stage right now, or is this just a way for you to say "we'll get around to it eventually, but it might take a while"?

Thanks,
(In reply to Wouter Verhelst from comment #13)
> I see you marked this as "P5". Does that mean we're at a "patches are
> welcome" stage right now, or is this just a way for you to say "we'll get
> around to it eventually, but it might take a while"?
> 

It's more the former than the latter. A P5 is likely never to make it onto the team's plate due to the number of bugs that are higher priority.
I don't think this is a good idea. PSM and it's dependency on PKCS#11 are legacy technologies we should be looking to move away from in the long run, and exposing those APIs to addons would significantly hinder doing so. (And we definitely shouldn't allow using this mechanism to load binary modules, for all the reasons we've been forbidding it elsewhere.)

I would suggest that a better approach would be to consider the use-cases people are seeing to enable, and expose that specific, minimal functionality in a way that doesn't tie us to the existing implementation.
This configuration can be done by going to about:preferences#privacy and clicking on Security Devices though? We've got a deprecation plan in place for APIs should we wish to remove them, so when Firefox decides to do that, we can also deprecate them. We haven't got a suggested API yet for the use cases yet. 

I would agree that loading binary modules would be bad, is that needed for PKCS#11 support?
Annie is a Belgian Citizen. She wants to declare her taxes. She knows that to do so, she needs to log on to the government's web site using her electronic ID card. She doesn't know how it all works though, but she doesn't really care. Annie opens her non-firefox browser, goes to https://eid.belgium.be/, downloads the software for her operating system, and installs it. Once installed, Annie uses her non-firefox browser, and logs on to the government's tax website using her eID.

Bernard is another Belgian citizen. His understanding of how the whole thing works is similar to Annie's, but the main difference is that Bernard uses Firefox 52. He goes to https://eid.belgium.be/, downloads the software for his operating system, and installs it. Once installed, Bernard uses his version of Firefox to browse to the government's tax website, but discovers that, since he's using Firefox, the website tells him that he *also* needs to install this add-on. He's mildly annoyed that there are two steps necessary; but after performing this second step, everything seems to work just fine.

Charlotte is a third Belgian Citizen, with a similar level of understanding. Charlotte uses Firefox 57. She goes to https://eid.belgium.be/, downloads the software for her operating system, and installs it. Once installed, Charlotte browses to the government website, and discovers that, since she's using the more modern version of Firefox, she needs to manually configure the eID module into her firefox. This involves going to the preferences screen, clicking on various obscure options, and entering an exact path name to a DLL file. Charlotte doesn't know what a DLL file is, but she tries to copy the correct name into the dialog box. Unfortunately, Charlotte is a bit confused, and enters the DLL filename in the field for the module name, and the module name in the field for the file name. Nothing works. After checking carefully, she moves the two fields around, but introduces a space in the filename field that shouldn't be there. Things still don't work. Charlotte gives up, and uses Safari instead. That works. After declaring her taxes, Charlotte complains to the Belgian government, because can't they make things easier?

Annie's use case is what currently happens for Internet Explorer, Microsoft Edge, Safari, and Chrome (on Windows and macOS; on Linux, this is a different situation, which involves command-line incantations that I won't go into). Bernard's use case is what currently happens for all versions of Firefox on all operating systems. It's theoretically possible to sideload the current addon with the installer (and we do this for Linux), but for various reasons there are sometimes problems with doing so, and therefore we don't sideload the Belgian eID addon on Windows or macOS.

If there is no way to programmatorically add PKCS#11 modules into Firefox anymore, then the most likely result will be Charlotte's experience. I would like to avoid that if at all possible, because 

PKCS#11 is a standard API which allows a cryptographic system to offload cryptographic operations to another software module (a dynamic library). This other software module can implement the said cryptographic operations in software, or it can pass it to a hardware device. In the case of a smartcard, such as the Belgian electronic ID card, the PKCS#11 module will pass any signature operations to the smartcard, since the private key is on the smart card and can never leave it. The whole point of having PKCS#11 support in firefox *is* so that it can load binary modules to do such operations. If we can talk to PKCS#11 modules but not load them, then there's no point and you might as well not bother (but then see Charlotte).

In the end, I don't really care how it's done. If Mozilla wants to drop support for PKCS#11 and implement something else instead, then that's fine and you should just do so. But until that's done, I would like to keep Bernard happy (and not turn his situation into Charlotte's).

For whatever reason, Mozilla has apparently decided that installing PKCS#11 modules from outside the browser is evil. Fine, whatever. I disagree, but that's not the point. However, please don't assume that every user who wants to use a smart card in combination with Firefox understands in detail how everything works, and please allow us to help them with an Annie or Bernard-style of user experience, rather than a Charlotte one.

I have been working on proposing an API to be able to do so in a webextensions context (but it's not quite ready yet), if that's the problem. But can I ask that anyone who objects to this situation please try their best to understand the situation first?

Thanks,
If I understand, you're having your users download and run an executable. If you know where the user's Firefox profile is, you should be able to use the NSS APIs to add your PKCS#11 module to their module database.

In general, though, PKCS#11 is not a great fit for the web. It attempts to assert identity at the network layer, rather than the application layer, which is the wrong thing in some cases (e.g. HTTP/2 connection coalescing). There's also privacy concerns (that can be mitigated, but basically no one does, as far as I know). For Firefox specifically, loading up 3rd party binary implementations in Firefox's memory space has historically led to a significantly degraded user experience (crashes, mainly). Something like Web Authentication (which we're working on) would relieve your users of these installation, configuration, and privacy issues, as well as stability concerns in Firefox.
IMHO for special applications, being able to authenticate on the web to known entities is a good thing. Tax or voting systems, with government issued tokens, are good examples.

Because there doesn't seem to be any kind of standard protocol for talking to the hardware itself, the common approach is for a vendor to provide a PKCS#11 driver on top of that, which is a binary module.

I think the argument "we forbid binary modules elsewhere" shouldn't be a sufficient argument to ban any kind of such modules. IMHO it makes a difference, if you allow some big external module to parse and display arbitrary content from the web (flash-like), or if you allow a small external module to calculate a checksum on some content, and do some local math to calculate a digital signature.

In other words, since PKCS#11 authentication signatures usually don't process and render web content, I don't think they need to be banned in general.

Also, depending on your hardware and operating system, the source code for the PKCS#11 modules might be open source.

So, I disagree with the suggestion that PKCS#11 are legacy technology in general, I think it would be good to keep this technology.

On the other hand, we know that downloading a binary module from the web, and installing it, can be very dangerous, given that once you run it with user permission, that module is no longer constrained by the web sandbox, and has access to the user's local data.

For this reason, it seems risky to allow web-initiated local installation to happen with little user interaction.

The installation of any software that runs outside the sandbox, including a PKCS#11 driver module for your hardware token, should remain a deliberate user decision. From my reading of the initial request in this bug, it seems that this is the required approach, the user must still install the software.

The request in this bug is to automatically configure the browser to load an external binary module, triggered by JavaScript that originates from the web.

I'm not convinced that's a good idea. I think the decision that web content should be able to execute a local binary, should require a deliberate action, too, to ensure the user has understood the potential consequences of that action.

I think the required steps, described in comment 0, are doable for end users, who really want to access the system.

If I'm wrong, and this is really considered too difficult for most users, then a different mechanism should be used. Given that you already convinced the user to install and run software to install the PKCS#11 driver, it seems reasonable to use that program to help with the additional required configuration.

How about defining a new file on the filesystem level, for example, a file that needs to be stored inside the Firefox user profile directory, e.g. using install-pkcs11-request-ARBITRARYSUFFIX.txt

Given the installed software has access to the user's files, it should be able to find the Firefox profile directory, and copy such a file into it (or into all those directories of the current user).

Upon start, Firefox could detect the presence of that file, either automatically, or using a more easily reached menu item, and offer the user to install it. If refused, the file is removed.
"How about defining a new file on the filesystem level, for example, a file that
needs to be stored inside the Firefox user profile directory, e.g. using
install-pkcs11-request-ARBITRARYSUFFIX.txt"

What about, when the external installer is executed before user profile? (Domains, OEM computers ...etc)
Why not use same solution like we have with loading WebExtensions, one global registry key to load on initial launch PKCS11 driver?
Hi Kai,

(In reply to Kai Engert (:kaie:) from comment #19)
> IMHO for special applications, being able to authenticate on the web to
> known entities is a good thing. Tax or voting systems, with government
> issued tokens, are good examples.
> 
> Because there doesn't seem to be any kind of standard protocol for talking
> to the hardware itself, the common approach is for a vendor to provide a
> PKCS#11 driver on top of that, which is a binary module.
> 
> I think the argument "we forbid binary modules elsewhere" shouldn't be a
> sufficient argument to ban any kind of such modules. IMHO it makes a
> difference, if you allow some big external module to parse and display
> arbitrary content from the web (flash-like), or if you allow a small
> external module to calculate a checksum on some content, and do some local
> math to calculate a digital signature.

+1

[...]
> For this reason, it seems risky to allow web-initiated local installation to
> happen with little user interaction.

Absolutely. The decision to install a module into the browser should *never* be done without user interaction; I agree that that is a disaster waiting to happen.

Note that "run an installer that requires administrator rights" is already some form of user interaction. Whether it's enough is, of course, open for debate.

> The installation of any software that runs outside the sandbox, including a
> PKCS#11 driver module for your hardware token, should remain a deliberate
> user decision. From my reading of the initial request in this bug, it seems
> that this is the required approach, the user must still install the software.

Indeed.

> The request in this bug is to automatically configure the browser to load an
> external binary module, triggered by JavaScript that originates from the web.

No, that's a misunderstanding.

Currently, Firefox allows addons to configure PKCS#11 modules from the addon. Content scripts cannot (and should not be allowed to) do so. Note that in times past, content scripts could do so, but that functionality has been removed, and rightly so; I'm not requesting that that is brought back.

The move to WebExtensions, however, means that addons will soon no longer be allowed to configure PKCS#11 modules. This bug is to request that some form of the functionality to configure PKCS#11 modules *from an addon context* is brought back. I have suggested to do so via WebExtensions, because that seems to be the most logical move forward given the history, but all I really care about is to avoid a Charlotte-style of user experience, as per comment 17.

> I'm not convinced that's a good idea. I think the decision that web content
> should be able to execute a local binary, should require a deliberate
> action, too, to ensure the user has understood the potential consequences of
> that action.

+1. I agree that web content should not be allowed to muck with PKCS#11 configuration. That's not what I'm requesting.

[...]
> How about defining a new file on the filesystem level, for example, a file
> that needs to be stored inside the Firefox user profile directory, e.g.
> using install-pkcs11-request-ARBITRARYSUFFIX.txt
> 
> Given the installed software has access to the user's files, it should be
> able to find the Firefox profile directory, and copy such a file into it (or
> into all those directories of the current user).
> 
> Upon start, Firefox could detect the presence of that file, either
> automatically, or using a more easily reached menu item, and offer the user
> to install it. If refused, the file is removed.

The problem with that, as Raul already pointed out, is that it assumes the user's profile has already been created at the time when the module is installed. This is not a given. Additionally, mucking about on the file system to find all potential firefox profile directories so as to be able to drop a file in them feels like a flakey and error-prone procedure to me (YMMV though).

However, I think the concept of a flag file to request the installation of a PKCS#11 module is not necessarily a bad idea, provided the flag file is installed in a system-wide location, rather than a per-user location. Firefox could then, upon first noticing the flag file, request confirmation from the user, and install the module if the user consents. In case the user does not consent, the correct action is probably to set some boolean in prefs.js, so that the user does not get bothered about it again, or something along those lines.

This gets rather close to the suggestion of having a system-wide module database as per bug#248722, though.
The base support for PKCS #11 has been in for 20 years. A more vendor friendly way of installing PKCS #11 modules was included in that initial work, but was removed long ago because it was based on Jar files.. Since that time The use of PKCS #11 in other systems has gotten better, but the UI work in Firefox has basically stalled. 

Currently your best bet is to bypass Firefox's UI and use direct NSS tools for manipulating the Firefox database. That is 'install' your PKCS #11 module independent of Firefox.

> "I don't think this is a good idea. PSM and it's dependency on PKCS#11 are legacy technologies we should be looking to move away from in the long run"
- PKCS #11 support in NSS is not going away anytime soon. Way to much stuff depends on it. It's use in the open source world continues. It would certainly be moving backwards to remove that support, particularly given the large deployment of smart card based authentication in Europe.
> If there is no way to programmatorically add PKCS#11 modules into Firefox anymore, then the most likely result will be 
> Charlotte's experience. I would like to avoid that if at all possible, because PKCS#11 is a standard API which allows a 
> cryptographic system to offload cryptographic operations to another software module (a dynamic library).

There is no way through the browser (AFAIK). You can build a standalone installer. A sample program which installs a PKCS #11 module can be found here: https://github.com/dogtagpki/coolkey/tree/master/src/install It requires NSS to run (I think you just need softoken and it's dependencies). It should run on Windows, Mac and Linux.
So do we have a verdict here?

It sounds like the PKCS #11 requirements aren't going away anytime soon, and teaching people to use some convoluted mechanism to add a DLL to their Firefox seems like the wrong idea.

You can do this via an installer, but that seems like a lot of work to solve this in a general way when it's been much easier in the past.

An API could be provided, but it would need to have strict permissions.

If this API were created, would we take it?
This has already been marked approved, I haven't seen anything yet that would dissuade me from accepting it while the feature exists in Firefox.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Like Wouter Verhelst and the eID Belgian Citizen, we have a governmental health smartcard used by more than 1.000.000 professionals. A great part of them will be impacted by Firefox 55 and next versions evolutions. We have Annie, Bernard and Charlotte profils too.
We are really concern by security issues because this is our job every day : secure access to private and personal health data.

Our solution are based on a specific installer for our PKCS11 library and specially for Firefox a XPI which is able to configure a Security Module to access to a SamrtCard. There is no lake of security if the PKCS11 library is installed by the final user with his explicit confirmation on the operating system process and in a second time, the configuration of Firefox is done by an extension inside your browser.
I've started some work on a webextension experiment that would implement the functionality requested in this bug over at https://github.com/yoe/pkcs11-enabler/

At the time of writing, it's very much a work in progress still, but already provides three functions:

- one to check whether a module has been installed by providing a name;
- one to check whether a module has been installed by providing a list of possible PKCS#11 library filenames;
- one to install a module by passing a name, a library filename, and some optional flags (which may not be necessary for every module, but is used by the current add-on for the beID PKCS#11 module)

I think this is the bare minimum for allowing PKCS#11 module providers to install their module using an add-on. However, I also believe that some extra functionality is useful:

- A way to allow removing a module when it is no longer needed; this seems like an obvious missing part for the code right now, and indeed I'm looking into how to do it.
- A way to allow verifying whether a slot and/or token are found by a given PKCS#11 module; this is helpful for debugging in case SSL authentication fails. The 403 page provided by the webserver could try to load a content script from the add-on; and if available, use that to ask the add-on whether slots or tokens are found. If not, a message "please insert your card and try again", or "please connect your card reader and try again" or something along those lines could be displayed.
- A way to allow objects to be read from the token. The Belgian eID PKCS#11 module provides access to the identity information on the card through CKO_DATA objects in the PKCS#11 module. It might be nice to allow websites access to that identity information this way. However, this could be a layering violation and a security risk; it may not be the best option.

Some feedback on whether the current state of things is looking like it's going to be acceptable down the road, and/or whether the three other things I'm looking at would be is welcome. In addition, since I'm not really well versed in javascript, comments on whether the current state of affairs is very javascript-y would be nice, too.
Flags: needinfo?(amckay)
I'm going to redirect you to Tomica, who has agreed to help out with this API.
Mentor: tomica
Flags: needinfo?(amckay) → needinfo?(tomica)
Flags: needinfo?(tomica)
Hi Wouter,

this has been approved, so I suggest you format and submit it directly as a patch for firefox, as there are slight differences between how code runs as part of firefox and as an experiment, which we can avoid.

(If you've never done that, please check out [1] and ask if you have any questions).

Once you are set up, please run eslint [2], it will tell you to use `let`/`const` instead of `var` for example.

And after that, please use mozreview [3] to submit your code for review.


From a quick glance, I see an issue with the `path` argument to the installModule() method.  Web extensions don't have access to local file system, so figuring out the exact path might not be possible.  Can you think of other approaches there?


1) https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction
2) https://developer.mozilla.org/en-US/docs/ESLint
3) https://reviewboard.mozilla.org/
(In reply to Tomislav Jovanovic :zombie from comment #29)
> From a quick glance, I see an issue with the `path` argument to the
> installModule() method.  Web extensions don't have access to local file
> system, so figuring out the exact path might not be possible.  Can you think
> of other approaches there?

I don't think that's a problem.

As far as Firefox is concerned, there are three ways in which a PKCS#11 module might end up on the filesystem:

- It could get shipped with the addon through addons.mozilla.org. The location will then obviously be passed by the addon as an addon-relative location. My experiment doesn't implement this, but the idea was to add that once I'd figured out how (which I forgot to mention in my previous message).
- It could be installed by an external installer, which will install it to a fixed location, or possibly a limited set of possible locations. These fixed locations would be hardcoded in the addon, which could produce an error message if trying to install the module fails for all of those hardcoded locations for this OS. This is, in fact, the approach that the Belgian eID addon takes currently; no attempt is made to check whether a module exists, other than that if installing the module fails, we assume the file isn't there and move on to the next possible location.
- The user could be provided with the PKCS#11 module through various means (e.g., an archive file, email attachment, or whatnot) and be expected to manually place it "somewhere" on their system. In this case, the addon cannot know where the addon might be. However, if the PKCS#11 installation is a manual process, I think it is not too much to ask to also make enabling the PKCS#11 module in firefox a manual process, through the currently-existing UI.
(In reply to Wouter Verhelst from comment #30)
> - The user could be provided with the PKCS#11 module through various means
> (e.g., an archive file, email attachment, or whatnot) and be expected to
> manually place it "somewhere" on their system. In this case, the addon
> cannot know where the addon might be.

This should obviously have been "the addon cannot know where the PKCS#11 module might be" :-)
We've discussed in the past having WebExtensions be able to READ registry entries (for various reasons).

I wonder if that would solve the location problem?
(In reply to Mike Kaply [:mkaply] from comment #33)
> We've discussed in the past having WebExtensions be able to READ registry
> entries (for various reasons).
> 
> I wonder if that would solve the location problem?

Don't think so, but maybe I don't understand the specifics.

As I've explained in comment 30, an addon is expected to know where the PKCS#11 module that it is interested in is supposed to be located; we expect that the addons are written by the developers of the PKCS#11 module themselves. There is no system-wide registry of PKCS#11 modules on Windows or OSX (there is libp11-kit on Linux, but trying to access that from WebExtensions seems like a bad idea). Given that, the only possible ways to detect PKCS#11 modules is "brute-force all .dll or .dylib files on the system" (not a very interesting approach) or "give the user a file selection box and ask them to point us to a file" (but then they might as well use the builtin UI for managing PKCS#11 modules).

The current legacy addons all have a hardcoded list of possible PKCS#11 module locations, and that approach seems to work. It does mean addons need to try them all in succession, though; it might be better to make installModule take an array of possible locations, and have the loop be run there, so that it doesn't require to be run from the Promise's .then() call. I'll look into that.

Unless I'm missing something (in which case, please clarify), I don't think having the ability to read things from some registry will help us very much.
Attachment #8886230 - Flags: review?(tomica)
Attachment #8886581 - Flags: review?(tomica)
(In reply to Wouter Verhelst :wouter from comment #30)
> - It could get shipped with the addon through addons.mozilla.org.

No, we will not be supporting shipping/signing binary modules from AMO.


> - It could be installed by an external installer, which will install it

This is the use case that we are targeting, as it provides the best balance between convenience and security.


> - The user could be provided with the PKCS#11 module through various means
> (e.g., an archive file, email attachment, or whatnot) and be expected to
> manually place it "somewhere" on their system. In this case, the addon
> cannot know where the addon might be. However, if the PKCS#11 installation
> is a manual process, I think it is not too much to ask to also make enabling
> the PKCS#11 module in firefox a manual process, through the
> currently-existing UI.

I'm not sure I understand this.  Is this use case already supported in firefox?
:andym, :dveditz and me had a brief discussion over irc last week, noting key takeaways here as a reminder:

 - This will need security review after the API is close to final.
 - Should ping Kai Engert (above) for feedback.
 - This will require a permission, but not a "special permission".
 - Binary component already installed by user, this API is just support for configuring.
 - The best way for installer to "configure firefox" is by installing a signed AMO extension for the user.
(In reply to Wouter Verhelst :wouter from comment #30)
> - It could be installed by an external installer, which will install it to a
> fixed location, or possibly a limited set of possible locations. These fixed
> locations would be hardcoded in the addon, which could produce an error
> message if trying to install the module fails for all of those hardcoded
> locations for this OS. This is, in fact, the approach that the Belgian eID
> addon takes currently; no attempt is made to check whether a module exists,
> other than that if installing the module fails, we assume the file isn't
> there and move on to the next possible location.

This blind poking at dll modules until we hit one still doesn't sit well with me, so I gave it some more though, and I think this is very similar to the situation we have with native messaging [1] where an application needs to provide a manifest that list extensions allowed to connect, and extension just names the application, Firefox checks the manifest, locates the application, and connects the two.

Can you please adapt your proposal to use a hosts manifest file, probably with a new type of "pkcs#11" or something that makes sense.

1) https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Native_messaging
Comment on attachment 8886230 [details]
Bug 1357391 - Implement a PKCS#11 management API

https://reviewboard.mozilla.org/r/157012/#review165980

Canceling review because of a different approach, but here are some code comments

::: commit-message-63441:1
(Diff revision 2)
> +Implement a PKCS#11 management API (bug 1357391).

Standard commit message format is:

  Bug 1357391: Implement a PKCS#11 management API r?zombie

  ...

::: toolkit/components/extensions/ext-pkcs11mod.js:3
(Diff revision 2)
> +"use strict";
> +
> +const moduledb = Components.classes["@mozilla.org/security/pkcs11moduledb;1"].getService(Components.interfaces.nsIPKCS11ModuleDB);

I think this can be loaded lazily: http://searchfox.org/mozilla-central/source/toolkit/components/extensions/ext-idle.js#8

::: toolkit/components/extensions/ext-pkcs11mod.js:5
(Diff revision 2)
> +"use strict";
> +
> +const moduledb = Components.classes["@mozilla.org/security/pkcs11moduledb;1"].getService(Components.interfaces.nsIPKCS11ModuleDB);
> +
> +this.pkcs11mod = class extends ExtensionAPI {

Is "mod" short for module?  Let's use a full name.

::: toolkit/components/extensions/ext-pkcs11mod.js:22
(Diff revision 2)
> +          let modules = moduledb.listModules();
> +          try {
> +            while (modules.hasMoreElements()) {
> +              let module = modules.getNext().QueryInterface(Components.interfaces.nsIPKCS11Module);
> +              if (module) {
> +                if (paths.indexOf(module.libName) !== -1) {

paths.contains()

::: toolkit/components/extensions/ext-pkcs11mod.js:34
(Diff revision 2)
> +            return false;
> +          }
> +        },
> +        async installModule(name, path, flags) {
> +          try {
> +            let addModule = Components.classes["@mozilla.org/security/pkcs11;1"].getService(Components.interfaces.nsIPKCS11).addModule;

We can also load this service at the top (lazily).
Attachment #8886230 - Flags: review?(tomica)
Comment on attachment 8886581 [details]
Add the ability to check slots

https://reviewboard.mozilla.org/r/157402/#review165462

::: commit-message-80079:1
(Diff revision 1)
> +Add the ability to check slots

Commit message format:

    Bug X - Part 2: Add some other thing r?zombie

::: toolkit/components/extensions/ext-pkcs11mod.js:54
(Diff revision 1)
>          },
> +        async getSlots(name) {
> +          try {
> +            let module = moduledb.findModuleByName(name).QueryInterface(Components.interfaces.nsIPKCS11Module);
> +            let rv = [];
> +            let slots = module.listSlots();

Is there some documentation of these module methods?
Attachment #8886581 - Flags: review?(tomica)
(In reply to Tomislav Jovanovic :zombie from comment #41)
> Commit message format:
> 
>     Bug X - Part 2: Add some other thing r?zombie

Actually, this looks like you are doing the github "add commits willy-nilly, squash them later" workflow, which isn't supported that well on mozreview.

Unless you have a good reason to land this in separate commits, this should all be in the same one, and just update (amend) it and push to review each time, mozreview provides a nice diff to review only changes.
(In reply to Tomislav Jovanovic :zombie from comment #37)
> (In reply to Wouter Verhelst :wouter from comment #30)
> > - It could get shipped with the addon through addons.mozilla.org.
> 
> No, we will not be supporting shipping/signing binary modules from AMO.

Fair enough. I just mentioned it, because it is a possibility, but I can perfectly understand the reasons for not wanting to do that.

> > - It could be installed by an external installer, which will install it
> 
> This is the use case that we are targeting, as it provides the best balance
> between convenience and security.

Exactly.

> > - The user could be provided with the PKCS#11 module through various means
> > (e.g., an archive file, email attachment, or whatnot) and be expected to
> > manually place it "somewhere" on their system. In this case, the addon
> > cannot know where the addon might be. However, if the PKCS#11 installation
> > is a manual process, I think it is not too much to ask to also make enabling
> > the PKCS#11 module in firefox a manual process, through the
> > currently-existing UI.
> 
> I'm not sure I understand this.  Is this use case already supported in
> firefox?

No, it is not, and the above was meant as an argument against supporting that use case :-)

That is, if you just ship binary files and expect the user to drop them "somewhere", it's not too weird that we're saying "in that case, our API can't help you".
(In reply to Tomislav Jovanovic :zombie from comment #42)
> Unless you have a good reason to land this in separate commits,

The idea was that the first commit contains the "this is absolutely needed in order to make anything at all work", whereas the second contains some extra "this might be nice to hav" functionality.
Hi,

Sorry about the late reply; I was on holiday for a while.

(In reply to Tomislav Jovanovic :zombie from comment #39)
> Can you please adapt your proposal to use a hosts manifest file, probably
> with a new type of "pkcs#11" or something that makes sense.
> 
> 1) https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Native_messaging

I've looked at this, and it looks like a reasonable approach to signal where the PKCS#11 module is located. There are two possible approaches to use that information:

1. Firefox could, upon seeing a manifest file, immediately proceed to loading the PKCS#11 module (optionally after confirming with the user). There would be no WebExtensions API, and no need for the programmer of the PKCS#11 module (who is typically a C programmer rather than a Javascript one) to write a firefox add-on.
2. Firefox could simply make the information in the manifest file available to the add-ons listed in the manifest file, and rely on the developer to write the required WebExtensions glue to load the module in question.

The advantage of the former is that it is easier for the PKCS#11 developer: she simply needs to write the manifest file, put the PKCS#11 module in the right location, and she's done. There is no need to understand both Javascript and whatever language the PKCS#11 module is written in (which will typically be C or C++). However, without a WebExtensions API, the proposed APIs in my second commit would not be available.

The advantage of the latter is that it allows for more flexibility by the PKCS#11 developer, and that (at first glance) it would seem to require less development time of the WebExtensions API developer (i.e., me) to implement. It's not clear to me where the logic to detect PKCS#11 module with the first method would go, whether it would make sense to interact with the user at that point, and it would require some UI design & development to go with it in order to make it interact with the user in a proper way if that's desired. As compared to the handful of Javascript API calls to WebExtensions, I expect a significantly larger development effort required to make this work well -- but I could be wrong, not having looked at this in much detail yet.

Thoughts?
Flags: needinfo?(tomica)
(In reply to Wouter Verhelst :wouter from comment #45)
> Hi,
> 
> Sorry about the late reply; I was on holiday for a while.
> 
> (In reply to Tomislav Jovanovic :zombie from comment #39)
> > Can you please adapt your proposal to use a hosts manifest file, probably
> > with a new type of "pkcs#11" or something that makes sense.
> > 
> > 1) https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Native_messaging
> 
> I've looked at this, and it looks like a reasonable approach to signal where
> the PKCS#11 module is located. There are two possible approaches to use that
> information:
> 
> 1. Firefox could, upon seeing a manifest file, immediately proceed to
> loading the PKCS#11 module (optionally after confirming with the user).
> There would be no WebExtensions API, and no need for the programmer of the
> PKCS#11 module (who is typically a C programmer rather than a Javascript
> one) to write a firefox add-on.
> 2. Firefox could simply make the information in the manifest file available
> to the add-ons listed in the manifest file, and rely on the developer to
> write the required WebExtensions glue to load the module in question.
> 
> The advantage of the former is that it is easier for the PKCS#11 developer:
> she simply needs to write the manifest file, put the PKCS#11 module in the
> right location, and she's done. There is no need to understand both
> Javascript and whatever language the PKCS#11 module is written in (which
> will typically be C or C++). However, without a WebExtensions API, the
> proposed APIs in my second commit would not be available.
> 
> The advantage of the latter is that it allows for more flexibility by the
> PKCS#11 developer, and that (at first glance) it would seem to require less
> development time of the WebExtensions API developer (i.e., me) to implement.
> It's not clear to me where the logic to detect PKCS#11 module with the first
> method would go, whether it would make sense to interact with the user at
> that point, and it would require some UI design & development to go with it
> in order to make it interact with the user in a proper way if that's
> desired. As compared to the handful of Javascript API calls to
> WebExtensions, I expect a significantly larger development effort required
> to make this work well -- but I could be wrong, not having looked at this in
> much detail yet.
> 
> Thoughts?

Giving this a little more thought, there might also be a middle ground:

3. Firefox could load the PKCS#11 module upon seeing the manifest file, and the proposed API in the second commit (the getSlots call) could be made available for those add-ons that are listed in the manifest file. This way, a web developer could still provide the diagnostic information as per the pseudocode in that second commit's message, and an add-on could still provide useful functionality if the PKCS#11 module developer wants to provide it.
We could install a loadable NSS module which automatically loads PKCS #11 modules from a given directory. The underlying NSS structure allows for that.
(In reply to Wouter Verhelst :wouter from comment #45)
> 2. Firefox could simply make the information in the manifest file available
> to the add-ons listed in the manifest file, and rely on the developer to
> write the required WebExtensions glue to load the module in question.

This was mostly what I had in mind -- though the location is not made available, the extension is only able to ask for and load modules by name, which map to manifest files.

This is how native messaging works, nothing is loaded "automatically" by firefox, only when requested by extension, and only by name and if all permissions line up.
Flags: needinfo?(tomica)
Attachment #8886581 - Attachment is obsolete: true
Comment on attachment 8886230 [details]
Bug 1357391 - Implement a PKCS#11 management API

https://reviewboard.mozilla.org/r/157012/#review165980

> Is "mod" short for module?  Let's use a full name.

I used "pkcs11" rather than "pkcs11mod" this time around. The idea was to keep the name "short"; the full name would be something like "pkcs11ModuleManagement", which is a bit excessive.

> paths.contains()

due to the change in how we find the path, there is now only one path, and therefore it's no longer an array, so the contains() operation is no longer necessary.
Comment on attachment 8886581 [details]
Add the ability to check slots

https://reviewboard.mozilla.org/r/157402/#review176386

::: toolkit/components/extensions/ext-pkcs11mod.js:54
(Diff revision 1)
>          },
> +        async getSlots(name) {
> +          try {
> +            let module = moduledb.findModuleByName(name).QueryInterface(Components.interfaces.nsIPKCS11Module);
> +            let rv = [];
> +            let slots = module.listSlots();

not that I know of. I just looked at the relevant IDL files (which are commented pretty extensively) and went through some trial-and-error to figure out how things work.
Note that bug 1391404 will have an effect on this experiment (hopefully by making it simpler).
(In reply to David Keeler [:keeler] (use needinfo?) from comment #52)
> Note that bug 1391404 will have an effect on this experiment (hopefully by
> making it simpler).

Yeah, looks like.

This will only affect the interface we use to talk to Firefox though, so if that gets in before this one, I won't need to change that much in my code. The critical bit here is what the external interface will look like, anyway.

Thanks for the heads up, at any rate.
(In reply to Wouter Verhelst :wouter from comment #51)
> not that I know of. I just looked at the relevant IDL files (which are
> commented pretty extensively) and went through some trial-and-error to
> figure out how things work.

Can you please point me to some of them, as those I looked at had no comments:
http://searchfox.org/mozilla-central/source/security/manager/ssl/nsIPKCS11Module.idl
(In reply to Tomislav Jovanovic :zombie from comment #54)
> (In reply to Wouter Verhelst :wouter from comment #51)
> > not that I know of. I just looked at the relevant IDL files (which are
> > commented pretty extensively) and went through some trial-and-error to
> > figure out how things work.
> 
> Can you please point me to some of them, as those I looked at had no
> comments:
> http://searchfox.org/mozilla-central/source/security/manager/ssl/
> nsIPKCS11Module.idl

Whoops, sorry -- you're right, they're not commented all that well. It's just that I remember going through them and being able to figure things out from there. I suppose it helps that I know how PKCS#11 is supposed to work, in general; I could imagine you might not be able to figure out things if you don't.

(there are some comments in nsIPK11Token.idl and nsIPKCS11Slot.idl, but they're not all that explanatory. Additionally, there's some comments in nsIPK11TokenDB.idl, but we don't use that)

Quick primer:

- A PKCS#11 module is loaded (through a scheme like dlopen() on Linux or macOS, and similar things on Windows).
- The PKCS#11 module allows the user access to something called "slots" in the PKCS#11 standard, which refer to a unit that may or may not have a "token". In the case of a PKCS#11 module in support of smartcards, a slot would correspond to a card reader.
- If a token is found in a slot (which in the case of a smartcard module would correspond to the smartcard that the module supports), then this token may or may not have one or more objects such as private keys, public keys, certificates, CA certificates, etc etc etc on it.

An application such as Firefox would access the PKCS#11 module, ask it for a list of slots, check which slots have tokens that the module supports[1], and then ask for a list of certificates and/or private keys on the token.

The idea of the "getSlots()" call in this proposed API would be to allow an extension to provide basic diagnostic information to the user in case authentication failed. The extension would be able to see that there is no smartcard in the reader, and tell the user to "please insert the smartcard, and make sure it's seated well and not upside down" or some such; or the extension could see that the module is found but that it doesn't find any slots, and tell the user to plug in their smartcard reader and/or install its driver.

[1] a user with two smartcards and one reader might be using the *other* smart card this time around.
Comment on attachment 8886230 [details]
Bug 1357391 - Implement a PKCS#11 management API

https://reviewboard.mozilla.org/r/157010/#review176850

A few questions and a lot of code comments, but the api design looks good to me.

I'll try to get a bit more familiar with the PKCS11 details before the next review, thanks for the primer.

This will also need tests before landing.

::: toolkit/components/extensions/NativeMessaging.jsm:177
(Diff revision 3)
>      this.readPromise = null;
>      this.sendQueue = [];
>      this.writePromise = null;
>      this.sentDisconnect = false;
>  
>      this.startupPromise = HostManifestManager.lookupApplication(application, context)

This logic below seems to be repeated, so let's extract it into a method like `HostManifestManager.readManifest(appName, type, context)` that would include the ckecks for `type` and `allowed_extensions`.

::: toolkit/components/extensions/NativeMessaging.jsm:181
(Diff revision 3)
>  
>      this.startupPromise = HostManifestManager.lookupApplication(application, context)
>        .then(hostInfo => {
> -        // Put the two errors together to not leak information about whether a native
> +        // Put the possible errors together to not leak information about whether a native
>          // application is installed to addons that do not have the right permission.
> -        if (!hostInfo || !hostInfo.manifest.allowed_extensions.includes(context.extension.id)) {
> +        if (!hostInfo || !hostInfo.manifest.allowed_extensions.includes(context.extension.id) || !(hostInfo.manifest.type === "stdio")) {

nit: !== instead of !(a === b)

::: toolkit/components/extensions/ext-pkcs11.js:4
(Diff revision 3)
> +"use strict";
> +
> +XPCOMUtils.defineLazyModuleGetters(this, {
> +        HostManifestManager: "resource://gre/modules/NativeMessaging.jsm",

nit: a single `defineLazyModuleGetter` is enough, and indent either 2 spaces, or align with the parens, like so:

http://searchfox.org/mozilla-central/source/toolkit/components/extensions/ExtensionCommon.jsm#23-33

::: toolkit/components/extensions/ext-pkcs11.js:12
(Diff revision 3)
> +XPCOMUtils.defineLazyServiceGetters(this, {
> +        pkcs11db: ["@mozilla.org/security/pkcs11moduledb;1", "nsIPKCS11ModuleDB"],
> +        pkcs11mgr:["@mozilla.org/security/pkcs11;1","nsIPKCS11"],
> +});
> +
> +const findModuleByPath = function(pkcs11db, path) {

nit: pkcs11db is global, no need to pass it every time

::: toolkit/components/extensions/ext-pkcs11.js:15
(Diff revision 3)
> +});
> +
> +const findModuleByPath = function(pkcs11db, path) {
> +  let modules = pkcs11db.listModules();
> +  while (modules.hasMoreElements()) {
> +    let module = modules.getNext().QueryInterface(Components.interfaces.nsIPKCS11Module);

nit: `Ci.nsIPKCS11Module`

::: toolkit/components/extensions/ext-pkcs11.js:17
(Diff revision 3)
> +const findModuleByPath = function(pkcs11db, path) {
> +  let modules = pkcs11db.listModules();
> +  while (modules.hasMoreElements()) {
> +    let module = modules.getNext().QueryInterface(Components.interfaces.nsIPKCS11Module);
> +    if (module) {
> +      if (module.libName === path) {

nit: can be one condition:

    if (module && module.libName === path) {

::: toolkit/components/extensions/ext-pkcs11.js:31
(Diff revision 3)
> +
> +  getAPI(context) {
> +    return {
> +      pkcs11: {
> +        async isInstalled(name) {
> +          return HostManifestManager.lookupApplication(name, context).then(hostInfo => {

await would be much more readable than callbacks here:

    let hostInfo = await HostManifestManager...
    if (!hostInfo) {
      ...

::: toolkit/components/extensions/ext-pkcs11.js:36
(Diff revision 3)
> +          return HostManifestManager.lookupApplication(name, context).then(hostInfo => {
> +            if(!hostInfo || !hostInfo.manifest.allowed_extensions.includes(context.extension.id) || !(hostInfo.manifest.type === "pkcs11")) {
> +              return false;
> +            }
> +            try {
> +              return (findModuleByPath(pkcs11db, hostInfo.manifest.path) !== null);

nit: parens not needed

::: toolkit/components/extensions/ext-pkcs11.js:44
(Diff revision 3)
> +            }
> +          });
> +        },
> +        async installModule(name, flags) {
> +          return HostManifestManager.lookupApplication(name, context).then(hostInfo => {
> +            let addModule = pkcs11mgr.addModule;

nit: doesn't achieve much

::: toolkit/components/extensions/ext-pkcs11.js:46
(Diff revision 3)
> +        },
> +        async installModule(name, flags) {
> +          return HostManifestManager.lookupApplication(name, context).then(hostInfo => {
> +            let addModule = pkcs11mgr.addModule;
> +            if(!hostInfo || !hostInfo.manifest.allowed_extensions.includes(context.extension.id) || !(hostInfo.manifest.type === "pkcs11")) {
> +              throw new context.cloneScope.Error(`This extension does not have permission to use native PKSC#11 module ${name} (or the module is not installed, or is not a PKCS#11 module)`);

this should be `return Promise.reject({message: "..."})`.

::: toolkit/components/extensions/ext-pkcs11.js:50
(Diff revision 3)
> +            if(!hostInfo || !hostInfo.manifest.allowed_extensions.includes(context.extension.id) || !(hostInfo.manifest.type === "pkcs11")) {
> +              throw new context.cloneScope.Error(`This extension does not have permission to use native PKSC#11 module ${name} (or the module is not installed, or is not a PKCS#11 module)`);
> +            }
> +            try {
> +              addModule(hostInfo.manifest.description, hostInfo.manifest.path, flags, 0);
> +              return true;

I think it's reasonable to assume most calls to this api should be successful, so let's just reject the promise (return Promise.reject..) instead of returning true/false.

::: toolkit/components/extensions/ext-pkcs11.js:60
(Diff revision 3)
> +        },
> +        async removeModule(name) {
> +          return HostManifestManager.lookupApplication(name, context).then(hostInfo => {
> +            let deleteModule = pkcs11mgr.deleteModule;
> +            if(!hostInfo || !hostInfo.manifest.allowed_extensions.includes(context.extension.id) || !(hostInfo.manifest.type === "pkcs11")) {
> +              throw new context.cloneScope.Error(`This extension does not have permission to use native PKCS#11 module ${name} (or the module is not installed, or is not a PKCS#11 module)`);

Let's define the error message in one place instead of repeating it.

::: toolkit/components/extensions/ext-pkcs11.js:70
(Diff revision 3)
> +            } catch (e) {
> +              return false;
> +            }
> +          });
> +        },
> +        async getSlots(name) {

How often is this method likely to be called.  I presume for others it would be on the order of once per extension startup, but if this is more often than that, it's probably better to cache the App Manifests (per extension/per module name).

Also, can you please add a significant comment describing what are "slots" and what this method is supposed to return.

::: toolkit/components/extensions/ext-pkcs11.js:83
(Diff revision 3)
> +              let slots = module.listSlots();
> +              while (slots.hasMoreElements()) {
> +                let slot = slots.getNext().QueryInterface(Components.interfaces.nsIPKCS11Slot);
> +                let token = slot.getToken().QueryInterface(Components.interfaces.nsIPK11Token);
> +                let slotobj = {};
> +                slotobj.name = slot.name;

Are slot names unique?  If so, this should probably be a {name: token} object instead of the array.

::: toolkit/components/extensions/schemas/pkcs11.json:10
(Diff revision 3)
> +      {
> +        "$extend": "Permission",
> +        "choices": [{
> +          "type": "string",
> +          "enum": [
> +            "pkcs11"

This permission will need a localization message in [1], so please write a concise explanation of the permission and ask Scott (sdevaney) in bugzilla to provide the final wording.

1) http://searchfox.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.properties#110

::: toolkit/components/extensions/schemas/pkcs11.json:50
(Diff revision 3)
> +            "type": "integer"
> +          }
> +        ]
> +      },
> +      {
> +        "name": "removeModule",

If the modules can be added and removed at runtime (without restarting), what about this naming scheme:

  isModuleLoaded()
  loadModule()
  unloadModule()
  getModuleSlots()
Attachment #8886230 - Flags: review?(tomica)
Comment on attachment 8886230 [details]
Bug 1357391 - Implement a PKCS#11 management API

You mentioned over irc that you left comments, but I don't see them in reviewboard.  Can you please double check that you did hit publish (it requires several confirmations, so you probably need to hit multiple buttons), or if not please copy/paste them into bugzilla..  :/
Attachment #8886230 - Flags: review?(tomica)
Comment on attachment 8886230 [details]
Bug 1357391 - Implement a PKCS#11 management API

https://reviewboard.mozilla.org/r/157012/#review178112
(In reply to Wouter Verhelst :wouter from comment #59)
> Comment on attachment 8886230 [details]
> Bug 1357391: Implement a PKCS#11 management API
> 
> https://reviewboard.mozilla.org/r/157012/#review178112

damn, so that didn't fix it.

If I go to the reviewboard, it doesn't show any "publish" button anymore, so I don't see how I can have it be published again...
(In reply to Tomislav Jovanovic :zombie from comment #58)
> Comment on attachment 8886230 [details]
> Bug 1357391: Implement a PKCS#11 management API
> 
> You mentioned over irc that you left comments, but I don't see them in
> reviewboard.  Can you please double check that you did hit publish (it
> requires several confirmations, so you probably need to hit multiple
> buttons), or if not please copy/paste them into bugzilla..  :/

So here's the comments I added:
	
176 lines

    this.startupPromise = HostManifestManager.lookupApplication(application, context)

	
259 lines
    An issue was opened. 
Show all issues

This logic below seems to be repeated, so let's extract it into a method like HostManifestManager.readManifest(appName, type, context) that would include the ckecks for type and allowed_extensions.

    Wouter Verhelst :wouter
    Wouter Verhelst :wouter
    23 hours, 50 minutes ago

        I tried that, but couldn't get it to work. lookupApplication is not an async function, but it does return a Promise, if I see it right. This code:

        readManifest(application, type, context) {
          return lookupApplication(application, context).then(hostInfo => {
            // Put the possible errors together to not leak information about whether a native
            // application is installed to addons that do not have the right permission.
            if(!hostInfo || !hostInfo.manifest.allowed_extensions.includes(context.extension.id) || hostInfo.manifest.type !== type) {
              return null;
            }
            return hostInfo;
        });


        added just below the "lookupApplication" function in NativeMessaging.jsm results in Firefox saying "lookupApplication is not defined" on that "return" line.

        I'm lost. Help?

        (I suppose changing "lookupApplication into an async function might help, but I'm not sure whether that might have side effects that we don't want, so not going down that route unless you tell me it's not a problem...)


	
69 lines

        async getSlots(name) {

	
37 lines
    The issue has been resolved. 
Show all issues

How often is this method likely to be called.  I presume for others it would be on the order of once per extension startup, but if this is more often than that, it's probably better to cache the App Manifests (per extension/per module name).

Also, can you please add a significant comment describing what are "slots" and what this method is supposed to return.

    Wouter Verhelst :wouter
    Wouter Verhelst :wouter
    23 hours, 51 minutes ago

        Cache added, although I'm not 100% sure that I've done so in the most optimal way. If there's a better way, let me know.

        Added the comment, although I suspect that authors of extensions that use this API will be the authors of the PKCS#11 modules and therefore know what slots are.


	
82 lines

                slotobj.name = slot.name;

	
24 lines
    The issue has been dropped. 
Show all issues

Are slot names unique?  If so, this should probably be a {name: token} object instead of the array.

    Wouter Verhelst :wouter
    Wouter Verhelst :wouter
    23 hours, 51 minutes ago

        In practice most likely yes, but it's not guaranteed and we can't rely on it.

        The most likely implementation of a PKCS#11 module for firefox would use PC/SC under the hood to deal with the low-level smartcard access. If that is the case, then they would be unique, because PC/SC uses the human readable name of a card reader as its identifier, adding a number (e.g., "MyCardreader #2") if necessary to distinguish two identical card readers from one another.

        However, there are other low-level smartcard access APIs, and the PKCS#11 API does not use the human-readable name as an identifier; instead, it uses a CK_ULONG (a typedef for unsigned long) as the unique identifier.

        In addition, PKCS#11 defines properties at the slot (as opposed to the token) level (e.g., the manufacturer of the card reader can be queried, or whether the reader has a physical keyboard for entering a PIN code). Since I don't think they are important for the purpose of installing a PKCS#11 module and/or diagnosing authentication errors, I didn't add them; however, a future update of this API might want to add one or more of these properties, and then having this be a {name: token} object would introduce either awkwardness or backwards incompatibilities.


	
49 lines

        "name": "removeModule",

	
25 lines
    The issue has been resolved. 
Show all issues

If the modules can be added and removed at runtime (without restarting), what about this naming scheme:

  isModuleLoaded()
  loadModule()
  unloadModule()
  getModuleSlots()

    Wouter Verhelst :wouter
    Wouter Verhelst :wouter
    23 hours, 51 minutes ago

        It is possible to add and remove a module at runtime at will, yes. However, calling addModule() on nsIPKCS11ModuleDB does more than just loading the module; it also registers it in the NSS database, so that the module will be loaded next time Firefox starts, even without the extension.

        Currently, most extensions do not bother removing the module when the extension is uninstalled from Firefox (possibly because there is no hook to do such cleanup? Not sure). In theory, it should be possible to only provide the installModule() call, but that just felt completely wrong :-)

        I've instead renamed them so they consistently use "install", i.e.:

        isModuleInstalled()
        installModule()
        uninstallModule()
        getModuleSlots()


        which seems more correct.

Other than those four comments, I did what you asked in your previous review and published a new review request
Flags: needinfo?(tomica)
>         added just below the "lookupApplication" function in
> NativeMessaging.jsm results in Firefox saying "lookupApplication is not
> defined" on that "return" line.

You need `this.lookupApplication()`, JS doesn't have implicit `this`.


>         Added the comment, although I suspect that authors of extensions
> that use this API will be the authors of the PKCS#11 modules and therefore
> know what slots are.

Yes, but this code needs to be maintained by firefox developers who might not be as familiar.


>         In practice most likely yes, but it's not guaranteed and we can't
> rely on it.

Fair enough.


>         I've instead renamed them so they consistently use "install"
>         which seems more correct.

Yeah, that sounds closer to the semantics described.
Flags: needinfo?(tomica)
Comment on attachment 8886230 [details]
Bug 1357391 - Implement a PKCS#11 management API

https://reviewboard.mozilla.org/r/157012/#review178316

::: browser/locales/en-US/chrome/browser/browser.properties:110
(Diff revision 4)
>  webextPerms.description.management=Monitor extension usage and manage themes
>  # LOCALIZATION NOTE (webextPerms.description.nativeMessaging)
>  # %S will be replaced with the name of the application
>  webextPerms.description.nativeMessaging=Exchange messages with programs other than %S
>  webextPerms.description.notifications=Display notifications to you
> +webextPerms.description.pkcs11=Manage PKCS#11 modules

This still needs final wording from Scott.

::: toolkit/components/extensions/NativeMessaging.jsm:182
(Diff revision 4)
>      this.startupPromise = HostManifestManager.lookupApplication(application, context)
>        .then(hostInfo => {
> -        // Put the two errors together to not leak information about whether a native
> +        // Put the possible errors together to not leak information about whether a native
>          // application is installed to addons that do not have the right permission.
> -        if (!hostInfo || !hostInfo.manifest.allowed_extensions.includes(context.extension.id)) {
> -          throw new context.cloneScope.Error(`This extension does not have permission to use native application ${application} (or the application is not installed)`);
> +        if (!hostInfo || !hostInfo.manifest.allowed_extensions.includes(context.extension.id) || !(hostInfo.manifest.type === "stdio")) {
> +          throw new context.cloneScope.Error(`This extension does not have permission to use native application ${application} (or the application is not installed, or is of the wrong type)`);

Let's keep the same message.

::: toolkit/components/extensions/ext-pkcs11.js:14
(Diff revision 4)
> +  "pkcs11db",
> +  "@mozilla.org/security/pkcs11moduledb;1",
> +  "nsIPKCS11ModuleDB"
> +);
> +
> +const {interfaces: Ci, utils: Cu} = Components;

this is already defined in the environment your code runs in.

::: toolkit/components/extensions/ext-pkcs11.js:33
(Diff revision 4)
> +  return `This extension does not have permission to use native PKSC#11 module ${name} (or the module is not installed, or is not a PKCS#11 module)`
> +}
> +
> +this.pkcs11 = class extends ExtensionAPI {
> +  getAPI(context) {
> +    let manifestCache = {};

Please use a `Map` instead.  The code below will fail to cache when `lookupApplication` return null.

::: toolkit/components/extensions/ext-pkcs11.js:38
(Diff revision 4)
> +    let manifestCache = {};
> +    let getManifest = async function(name) {
> +      if(!manifestCache[name]) {
> +        let hostInfo = await HostManifestManager.lookupApplication(name, context);
> +        if(hostInfo && hostInfo.manifest.allowed_extensions.includes(context.extension.id) && hostInfo.manifest.type === "pkcs11") {
> +          manifestCache[name] = await HostManifestManager.lookupApplication(name, context);

No need to lookup again here.  Also, might as well return `hostInfo.manifest` directly here.

::: toolkit/components/extensions/ext-pkcs11.js:48
(Diff revision 4)
> +    return {
> +      pkcs11: {
> +        async isModuleInstalled(name) {
> +          let hostInfo = await getManifest(name);
> +          if(!hostInfo) {
> +            return false;

`hostInfo` might be falsy because the extension is denied access (even if the module is installed), so I think rejecting here as well would be correct.  Which means the `getManifest()` method itself could always reject with the error message (which `await` will propagate out).

::: toolkit/components/extensions/ext-pkcs11.js:52
(Diff revision 4)
> +          if(!hostInfo) {
> +            return false;
> +          }
> +          try {
> +            return findModuleByPath(hostInfo.manifest.path) !== null;
> +          } catch (e) {

What kind of errors are you expecting here?  Perhaps handle them inside the method?

::: toolkit/components/extensions/ext-pkcs11.js:64
(Diff revision 4)
> +            return Promise.reject({message: errormsg`${name}`});
> +          }
> +          try {
> +            pkcs11db.addModule(hostInfo.manifest.description, hostInfo.manifest.path, flags, 0);
> +          } catch(e) {
> +            return Promise.reject({message: "Installing the module failed: " + e});

We try to only expose specific, expected exceptions to extensions, not all internal exceptions.

If there are some common and specific exceptions that would help diagnose the problems, you should catch them explicitly.  Otherwise, reject with a generic "didn't work" message.

(and perhaps logging the error via `Cu.reportError()` to the browser console might be appropriate)

::: toolkit/components/extensions/ext-pkcs11.js:75
(Diff revision 4)
> +            return Promise.reject({message: errormsg`${name}`});
> +          }
> +          try {
> +            deleteModule(findModuleByPath(hostInfo.manifest.path).name);
> +          } catch (e) {
> +            return Promise.reject({message: "Removing the module failed: " + e});

ditto

::: toolkit/components/extensions/ext-pkcs11.js:116
(Diff revision 4)
> +              slotobj.name = slot.name;
> +              slotobj.token = null;
> +              if(slot.status != 1 /* SLOT_NOT_PRESENT */) {
> +                slotobj.token = {};
> +                slotobj.token.name = token.tokenName;
> +                slotobj.token.desc = token.tokenLabel;

`description`

::: toolkit/components/extensions/ext-pkcs11.js:128
(Diff revision 4)
> +              rv.push(slotobj);
> +            }
> +            return rv;
> +          } catch (e) {
> +            Cu.reportError(e);
> +            return false;

Please reject instead of returning false.
Also, sorry but I just noticed this will only work on desktop firefox (and not on android), which means all the files should probably be in browser/ not toolkit/.

http://searchfox.org/mozilla-central/source/browser/components/extensions
(In reply to Wouter Verhelst :wouter from comment #65)
> Comment on attachment 8886230 [details]
> Bug 1357391: Implement a PKCS#11 management API
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/157012/diff/4-5/

please ignore that, this is so the file move doesn't make the interdiff useless, but there are no actual changes here.
Scott, please suggest a proper description for the new "pkcs11" permission (and possibly for the permission itself) that's needed for this bug.

The new permission would allow an add-on to manage (add, remove, inquire about) PKCS#11 modules that can be used for cryptographic authentication at the TLS layer (most likely by way of a smartcard, but PKCS#11 can theoretically be used for other purposes too).

Currently, I added:

webextPerms.description.pkcs11=Manage PKCS#11 modules

Thanks,
Flags: needinfo?(sdevaney)
Attachment #8886230 - Flags: review?(tomica)
Comment on attachment 8886230 [details]
Bug 1357391 - Implement a PKCS#11 management API

https://reviewboard.mozilla.org/r/157012/#review178316

> This still needs final wording from Scott.

Added a message to the bugreport and placed a needinfo? flag, so hopefully he'll get around to that. Keeping the issue open here, until Scott replies.

> Please use a `Map` instead.  The code below will fail to cache when `lookupApplication` return null.

The failure to cache there is intentional :-)

If we cache the absense of a manifest, then failing to install a module (because it wasn't installed yet when the add-on is first run) will cause it to be impossible to install it until the next time firefox is restarted. That doesn't seem right; I could imagine a user wanting to install an addon to install support for their smartcard, being told that they also need to install that external module there, and then being told that it won't work until they restart their firefox -- I think it's better not to do that.

Changed it so it uses a Map now.

> No need to lookup again here.  Also, might as well return `hostInfo.manifest` directly here.

if returning it directly, then we have two lines saying "return something", when that isn't necessary (you still need it in the "it's cached already" case), which feels a bit wrong to me if we can avoid it.

> What kind of errors are you expecting here?  Perhaps handle them inside the method?

In order to implement this "get module by module path" method, we're iterating over all the PKCS\#11 modules, including those that are not part of our manifest. If something up the stack fails, or the internals of how to deal with PKCS\#11 are changed at some point so that we no longer have permissions to see them, or something along those lines, it feels safer to deal with that by returning "it's not installed" and saying "nothing happened, please move on" rather than exposing the specific exception for possibly an unrelated module to the extension and thereby possibly leaking what that other PKCS\#11 module might be.

However, to aid in debugging, I've also added a reportError() here so that people can at least figure out what's going wrong.

> We try to only expose specific, expected exceptions to extensions, not all internal exceptions.
> 
> If there are some common and specific exceptions that would help diagnose the problems, you should catch them explicitly.  Otherwise, reject with a generic "didn't work" message.
> 
> (and perhaps logging the error via `Cu.reportError()` to the browser console might be appropriate)

The implementation of the "addModule" method apparently returns "NS_ERROR_NOT_AVAILABLE" when it is called while firefox is being closed, NS_ERROR_INVALID_ARG when the module name is empty, and NS_ERROR_FAILURE in all other cases. The same is true for the "deleteModule" call.

In that light, it doesn't seem to make much sense to catch them specifically. I added code to prevent the name being empty (which I mapped to the "description" field in the manifest), but beyond that we can only return a generic "failed to install" message, as you suggest.
Oof. There's no way to make this little piggy all pretty. I huddled with some engineers and this is the clearest we could come up with:

Provide cryptographic authentication services
Flags: needinfo?(sdevaney)
Comment on attachment 8886230 [details]
Bug 1357391 - Implement a PKCS#11 management API

Canceling review, Wouter plans to update the patch.
Attachment #8886230 - Flags: review?(tomica)
Assignee: nobody → w
Depends on: 1396030
Not sure why this is made blocking on some vapourware code?

I agree that moving PKCS#11 out-of-process is a good move, and that it will improve the security situation.

However, I don't think that adding a user-friendly way to install PKCS#11 modules will do much to make that situation worse. PKCS#11 modules can be loaded with our without this addition (although this API will allow adding them to be made much more userfriendly).

Please reconsider.
Hi David,

reading from your bug 1396030 comment #1, it seems that is something we definitely want to do, but is somewhat independent from this bug, and perhaps it shouldn't block this api from landing in time for 57?

(perhaps see comment #38 as a short TL;DR for the bug)

Also, can you please advise on what would be the recommended practice for adding test for something like this?  Can/should we have a stub/dummy module land as C++ source code (which gets built along with firefox) to at least smoketest the api, or does something like that already exist perhaps?
Forgot to ni?
Flags: needinfo?(dkeeler)
Comment on attachment 8886230 [details]
Bug 1357391 - Implement a PKCS#11 management API

https://reviewboard.mozilla.org/r/157012/#review181542

This looks good, thanks.  A few more nits, and we still need tests, but I think we are ready to ask for feedback from others while we figure that out.

::: browser/components/extensions/ext-pkcs11.js:29
(Diff revision 7)
> +
> +this.pkcs11 = class extends ExtensionAPI {
> +  getAPI(context) {
> +    let manifestCache = new Map();
> +    let getManifest = async function(name) {
> +      if(!manifestCache.has(name)) {

Space after `if`

::: browser/components/extensions/ext-pkcs11.js:32
(Diff revision 7)
> +    let manifestCache = new Map();
> +    let getManifest = async function(name) {
> +      if(!manifestCache.has(name)) {
> +        let hostInfo = await HostManifestManager.readManifest(name, context, "pkcs11");
> +        if(hostInfo) {
> +          manifestCache.set(name, hostInfo);

Sorry for badly worded comment list time, I meant can you just store `hostinfo.manifest` here, as that's all we need, everywhere.

::: browser/components/extensions/ext-pkcs11.js:34
(Diff revision 7)
> +      if(!manifestCache.has(name)) {
> +        let hostInfo = await HostManifestManager.readManifest(name, context, "pkcs11");
> +        if(hostInfo) {
> +          manifestCache.set(name, hostInfo);
> +        } else {
> +          return Promise.reject(`This extension does not have permission to use native PKCS#11 module ${name} (or the module is not installed, or is not a PKCS#11 module)`);

`.reject({message: "...`

also, don't think we need the the last part, you already call it a "PKCS#11 module".

::: browser/components/extensions/ext-pkcs11.js:52
(Diff revision 7)
> +            return false;
> +          }
> +        },
> +        async installModule(name, flags) {
> +          let hostInfo = await getManifest(name);
> +          if (hostInfo.manifest.description === null) {

I don't think this can ever be null if the manifest was loaded successfully, the `native_host_manifest.json` defines it as a non-optional string property.

Perhaps you meant to check if it's an empty string?  In any case, `if (!manifest.description)` would work for that`.

::: browser/components/extensions/ext-pkcs11.js:69
(Diff revision 7)
> +          let module = findModuleByPath(hostInfo.manifest.path);
> +          if(!module) {
> +            return Promise.reject({message: `The PKCS#11 module ${name} is not loaded`});
> +          }
> +          try {
> +            pkcs11db.deleteModule(findModuleByPath(hostInfo.manifest.path).name);

You already have the `module` here.

::: browser/components/extensions/schemas/pkcs11.json:46
(Diff revision 7)
> +            "type": "string"
> +          },
> +          {
> +            "name": "flags",
> +            "type": "integer"
> +          }

is this mandatory, or can it be an optional param?

::: toolkit/components/extensions/NativeMessaging.jsm:155
(Diff revision 7)
>      if (!VALID_APPLICATION.test(application)) {
>        throw new Error(`Invalid application "${application}"`);
>      }
>      return this.init().then(() => this._lookup(application, context));
>    },
> +

Can you please add jsdoc above the method describing it?

::: toolkit/components/extensions/NativeMessaging.jsm:156
(Diff revision 7)
>        throw new Error(`Invalid application "${application}"`);
>      }
>      return this.init().then(() => this._lookup(application, context));
>    },
> +
> +  readManifest(application, context, type) {

This can could be easier to read as an async method.

::: toolkit/components/extensions/NativeMessaging.jsm:190
(Diff revision 7)
>      this.writePromise = null;
>      this.sentDisconnect = false;
>  
> -    this.startupPromise = HostManifestManager.lookupApplication(application, context)
> +    this.startupPromise = HostManifestManager.readManifest(application, context, "stdio")
>        .then(hostInfo => {
> -        // Put the two errors together to not leak information about whether a native
> +        if(!hostInfo) {

Again space after `if`.  Please run eslint.

::: toolkit/components/extensions/NativeMessaging.jsm
(Diff revision 7)
> -        // Put the two errors together to not leak information about whether a native
> +        if(!hostInfo) {
> -        // application is installed to addons that do not have the right permission.
> -        if (!hostInfo || !hostInfo.manifest.allowed_extensions.includes(context.extension.id)) {
>            throw new context.cloneScope.Error(`This extension does not have permission to use native application ${application} (or the application is not installed)`);
>          }
> -

nit: no unrelated changes please
Attachment #8886230 - Flags: review?(tomica) → review+
Comment on attachment 8886230 [details]
Bug 1357391 - Implement a PKCS#11 management API

https://reviewboard.mozilla.org/r/157012/#review181602

::: browser/components/extensions/ext-pkcs11.js:63
(Diff revision 7)
> +            Cu.reportError(e);
> +            return Promise.reject({message: `Failed to install PKCS#11 module ${name}`});
> +          }
> +        },
> +        async uninstallModule(name) {
> +          let hostInfo = await getManifest(name);

Thanks to Andy for pointing me to take a second look.

`getManifest()` call will throw if the extension tries to unload a module that it doesn't have permission to, but with the right manifest, this method could be allowed to uninstall internal Firefox modules?  

Not sure that has security implications worse/different from installing a new module, but if we can easily prevent it, it's probably worth doing.
Hi Kai, Daniel (and others following the bug)

I think we have a solid proposal for the api and implementation (tests pending), so can you please take a look and provide feedback / start the security review process?


Comment 38 has brief takeaways from the last time we discussed the bug with Daniel, the patch commit message and the JSON schema have an overview of the API, and all other decisions were made in the bug, but since this is already at 77 comments, feel free to ask me or Wouter for summaries/explanations.
Flags: needinfo?(kaie)
Flags: needinfo?(dveditz)
(In reply to Tomislav Jovanovic :zombie from comment #77)
> Comment on attachment 8886230 [details]
> Bug 1357391: Implement a PKCS#11 management API
> 
> https://reviewboard.mozilla.org/r/157012/#review181602
> 
> ::: browser/components/extensions/ext-pkcs11.js:63
> (Diff revision 7)
> > +            Cu.reportError(e);
> > +            return Promise.reject({message: `Failed to install PKCS#11 module ${name}`});
> > +          }
> > +        },
> > +        async uninstallModule(name) {
> > +          let hostInfo = await getManifest(name);
> 
> Thanks to Andy for pointing me to take a second look.
> 
> `getManifest()` call will throw if the extension tries to unload a module
> that it doesn't have permission to, but with the right manifest, this method
> could be allowed to uninstall internal Firefox modules?  

That's a good point. Not sure how to detect whether or not the module is firefox-internal, though.

> Not sure that has security implications worse/different from installing a
> new module, but if we can easily prevent it, it's probably worth doing.

Sure
Comment on attachment 8886230 [details]
Bug 1357391 - Implement a PKCS#11 management API

https://reviewboard.mozilla.org/r/157012/#review181542

> is this mandatory, or can it be an optional param?

Honestly, I'm not entirely sure.

The XPCOM method requires it. If you don't specify any value, adding our particular module will fail. Traditionally, we've been specifying 0x1 << 28, which is PKCS11_PUB_READABLE_CERT_FLAG, and which states that certificates can be read from the card without being logged in.

I'm sure other PKCS#11 modules might need other values.

Having said all that, we could default it to being zero, I suppose.

> nit: no unrelated changes please

The comment is moved to inside the readManifest function. I also think it's better to use it at this location, since otherwise you have multiple places with the same test -- the avoidance of which is the whole purpose of creating that readManifest function.

I suppose I can drop it, though.
Comment on attachment 8886230 [details]
Bug 1357391 - Implement a PKCS#11 management API

https://reviewboard.mozilla.org/r/157012/#review181542

> The comment is moved to inside the readManifest function. I also think it's better to use it at this location, since otherwise you have multiple places with the same test -- the avoidance of which is the whole purpose of creating that readManifest function.
> 
> I suppose I can drop it, though.

That was about the removal of a blank line.  I should really be more explicit when commenting, sorry.
So, I wrote...

(In reply to Wouter Verhelst :wouter from comment #72)
> Not sure why this is made blocking on some vapourware code?

... which was instigated by the desire to see this be part of Firefox 57 and the sudden and (to me) unexpected arrival of a blocking bug which might make this be postponed and therefore not make it in time anymore.

I probably should have let this rest before replying, but I didn't, and, well.

Apologies to anyone whom I might have offended by a poor choice of words.
Comment on attachment 8886230 [details]
Bug 1357391 - Implement a PKCS#11 management API

https://reviewboard.mozilla.org/r/157012/#review181542

> That was about the removal of a blank line.  I should really be more explicit when commenting, sorry.

Oh, right. Missed that, sorry. I guess that happened due to me copying code around a few times, and being sloppy that one time.

Fixed for the next iteration.
Comment on attachment 8886230 [details]
Bug 1357391 - Implement a PKCS#11 management API

https://reviewboard.mozilla.org/r/157012/#review181602

> Thanks to Andy for pointing me to take a second look.
> 
> `getManifest()` call will throw if the extension tries to unload a module that it doesn't have permission to, but with the right manifest, this method could be allowed to uninstall internal Firefox modules?  
> 
> Not sure that has security implications worse/different from installing a new module, but if we can easily prevent it, it's probably worth doing.

Gave this some more thought, and while I can't really think of a security implication of an extension removing the wrong module, I *can* think of practical implications of an extension removing a Firefox-internal module; I don't believe it is possible for Firefox-internal module to be reinstalled once it has been removed, so that would require that the user's NSS database be recreated from scratch, which may result in loss of data. I didn't actually check this part though, so I might be wrong.

Additionally, an extension which messes around with other extensions' PKCS#11 modules could theoretically cause similar havoc, too. I can't see of any way to fix this, other than having Firefox register which module was installed by which extension, and refusing the uninstall operation unless the install operation was performed by the *same* extension (however, the extension should still be allowed to request information on the module even if it didn't install it, i.e., whether it *is* installed and getting the list of slots if it is). That might be too much effort for too little gain, though.

It might be that Firefox forbids the removal of Firefox-internal PKCS#11 modules, in which case your concern is covered. Someone more familiar with the internals of NSS should probably comment on that, though. Even so, that does not cover extensions messing with other extensions' modules, but we could decide to ignore that, or possibly it is something that extension review might be able to catch (I don't know).

The easiest way to fix this is probably to completely drop the uninstallModule() call, at least until a proper way to handle this issue can be worked out. I added it because IMO the API would feel somewhat incomplete if you can add something but not remove it again; but AFAICS there are no hooks that are executed at extension uninstall time--and I don't think there could be, since a sideloaded extension could be uninstalled by an operating system-level uninstall of the same sideloaded file. As such, there isn't really a useful point where an "uninstallModule()" call could be added, and so I don't *actually* believe anyone would miss the lack of this call. Still, there might be reasons that I'm missing, and again, the API would feel incomplete if you could add a module but then not remove it again.
(In reply to Tomislav Jovanovic :zombie from comment #74)
> reading from your bug 1396030 comment #1, it seems that is something we
> definitely want to do, but is somewhat independent from this bug, and
> perhaps it shouldn't block this api from landing in time for 57?

I'm going to defer to the module system here. I'm not part of the decision-making structure for WebExtensions, so it's not my call. I will say that given that the engineering work for bug 1396030 is likely to be done by security engineering (where I am part of the decision-making structure), I doubt we have the engineering resources or time to implement bug 1396030 for 57.

> Also, can you please advise on what would be the recommended practice for
> adding test for something like this?  Can/should we have a stub/dummy module
> land as C++ source code (which gets built along with firefox) to at least
> smoketest the api, or does something like that already exist perhaps?

We do have a test module. See https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/unit/pkcs11testmodule
There are some xpcshell tests that use it: https://dxr.mozilla.org/mozilla-central/search?q=path%3Asecurity%2Fmanager%2Fssl%2Ftests%2Funit+path%3Apkcs11+ext%3Ajs&redirect=false
It should be available to other test suites, but I'm not certain.

(In reply to Wouter Verhelst :wouter from comment #84)
> Additionally, an extension which messes around with other extensions'
> PKCS#11 modules could theoretically cause similar havoc, too. I can't see of
> any way to fix this, other than having Firefox register which module was
> installed by which extension, and refusing the uninstall operation unless
> the install operation was performed by the *same* extension (however, the
> extension should still be allowed to request information on the module even
> if it didn't install it, i.e., whether it *is* installed and getting the
> list of slots if it is). That might be too much effort for too little gain,
> though.

I think this is a good idea - an add-on should only be able to remove a module if it installed that module. If something goes wrong, the user can always use the built-in device manager to remove a module.

> It might be that Firefox forbids the removal of Firefox-internal PKCS#11
> modules, in which case your concern is covered.

Removing the internal module will fail in a safe way (you can try it yourself in the device manager). The larger concern is removing the built-in CA list. We should ensure that add-ons can't do this (see also bug 1391062).
Flags: needinfo?(dkeeler)
(In reply to David Keeler [:keeler] (use needinfo?) from comment #85)
> (In reply to Tomislav Jovanovic :zombie from comment #74)
> > reading from your bug 1396030 comment #1, it seems that is something we
> > definitely want to do, but is somewhat independent from this bug, and
> > perhaps it shouldn't block this api from landing in time for 57?
> 
> I'm going to defer to the module system here. I'm not part of the
> decision-making structure for WebExtensions, so it's not my call. I will say
> that given that the engineering work for bug 1396030 is likely to be done by
> security engineering (where I am part of the decision-making structure), I
> doubt we have the engineering resources or time to implement bug 1396030 for
> 57.
> 
> > Also, can you please advise on what would be the recommended practice for
> > adding test for something like this?  Can/should we have a stub/dummy module
> > land as C++ source code (which gets built along with firefox) to at least
> > smoketest the api, or does something like that already exist perhaps?
> 
> We do have a test module. See
> https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/
> unit/pkcs11testmodule
> There are some xpcshell tests that use it:
> https://dxr.mozilla.org/mozilla-central/
> search?q=path%3Asecurity%2Fmanager%2Fssl%2Ftests%2Funit+path%3Apkcs11+ext%3Aj
> s&redirect=false
> It should be available to other test suites, but I'm not certain.

I managed to get it to work in the first stab at tests that I just pushed, but I'm not sure I'm doing it in the best possible way.

> (In reply to Wouter Verhelst :wouter from comment #84)
> > Additionally, an extension which messes around with other extensions'
> > PKCS#11 modules could theoretically cause similar havoc, too. I can't see of
> > any way to fix this, other than having Firefox register which module was
> > installed by which extension, and refusing the uninstall operation unless
> > the install operation was performed by the *same* extension (however, the
> > extension should still be allowed to request information on the module even
> > if it didn't install it, i.e., whether it *is* installed and getting the
> > list of slots if it is). That might be too much effort for too little gain,
> > though.
> 
> I think this is a good idea - an add-on should only be able to remove a
> module if it installed that module. If something goes wrong, the user can
> always use the built-in device manager to remove a module.

That only begs the question of "how". Does Firefox have a way to register something in ways that cannot be (easily) altered by native installers? After all, if we're trying to defend against a native installer writing a malicious manifest file which breaks other people's modules, then we should also try to defend against native installers trying to modify whatever registry of "which add-on installed which module" we end up using, otherwise what's the point.

I think this quickly leads down a rabbit hole that I don't see an exit from, however.

> > It might be that Firefox forbids the removal of Firefox-internal PKCS#11
> > modules, in which case your concern is covered.
> 
> Removing the internal module will fail in a safe way (you can try it
> yourself in the device manager). The larger concern is removing the built-in
> CA list. We should ensure that add-ons can't do this (see also bug 1391062).

Right. I think in the long run the XPCOM code should filter this out, but in the next patch I'll explicitly filter out the nssckbi module in the WebExtensions code.
Flags: needinfo?(dkeeler)
Comment on attachment 8886230 [details]
Bug 1357391 - Implement a PKCS#11 management API

https://reviewboard.mozilla.org/r/157012/#review183918

::: browser/components/extensions/test/xpcshell/test_ext_pkcs11_management.js:29
(Diff revision 8)
> +async function setupManifests(modules) {
> +  async function writeManifest(module) {
> +    let manifest = {
> +      name: module.name,
> +      description: module.description,
> +      path: "../../../../../security/manager/ssl/tests/unit/pkcs11testmodule/" + ctypes.libraryName("pkcs11testmodule"),

I was hoping this would be available in some kind of common test utilities/libraries directory, but I guess it's not. You could see about filing a bug to do this, but I don't know that it's all that essential (if it moves or this moves, the test will break and we'll just have to update the path).

::: browser/components/extensions/test/xpcshell/test_ext_pkcs11_management.js:102
(Diff revision 8)
> +      return browser.pkcs11.isModuleInstalled("testmodule");
> +    }).then(result => {
> +      return browser.test.assertFalse(result, "PKCS#11 module is no longer installed after we uninstall it");
> +    }).then(() => {
> +      return browser.pkcs11.isModuleInstalled("othermodule").catch(invalidModuleError => {
> +        browser.test.assertTrue(invalidModuleError.message.includes("This extension does not have permission to use native PKCS#11 module othermodule (or the module is not installed)"), "We cannot access modules if we're not listed in the module's manifest file's allowed_extensions key");

style nit: this line is a bit long.

::: browser/components/extensions/test/xpcshell/xpcshell.ini:23
(Diff revision 8)
>  
>  [test_ext_manifest_commands.js]
>  [test_ext_manifest_omnibox.js]
>  [test_ext_manifest_permissions.js]
>  [test_ext_geckoProfiler_schema.js]
> +[test_ext_pkcs11_management.js]

I believe the test module isn't available on android, so you'll need a `skip-if = toolkit == 'android'`
(In reply to Wouter Verhelst :wouter from comment #87)
...
> I managed to get it to work in the first stab at tests that I just pushed,
> but I'm not sure I'm doing it in the best possible way.

I added some comments on the test. Looks good in general.

> > I think this is a good idea - an add-on should only be able to remove a
> > module if it installed that module. If something goes wrong, the user can
> > always use the built-in device manager to remove a module.
> 
> That only begs the question of "how". Does Firefox have a way to register
> something in ways that cannot be (easily) altered by native installers?
> After all, if we're trying to defend against a native installer writing a
> malicious manifest file which breaks other people's modules, then we should
> also try to defend against native installers trying to modify whatever
> registry of "which add-on installed which module" we end up using, otherwise
> what's the point.

I don't think we can defend against native installers here, but I don't think we're trying to, either. I think we're trying to prevent add-on A (installed from the web) from removing pkcs#11 entries added by add-on B (also installed from the web).

> > > It might be that Firefox forbids the removal of Firefox-internal PKCS#11
> > > modules, in which case your concern is covered.
> > 
> > Removing the internal module will fail in a safe way (you can try it
> > yourself in the device manager). The larger concern is removing the built-in
> > CA list. We should ensure that add-ons can't do this (see also bug 1391062).
> 
> Right. I think in the long run the XPCOM code should filter this out, but in
> the next patch I'll explicitly filter out the nssckbi module in the
> WebExtensions code.

Sounds good - add a test?
Flags: needinfo?(dkeeler)
Comment on attachment 8886230 [details]
Bug 1357391 - Implement a PKCS#11 management API

https://reviewboard.mozilla.org/r/157012/#review183918

> I believe the test module isn't available on android, so you'll need a `skip-if = toolkit == 'android'`

isn't this already covered by the fact that ext-pkcs11.js is in browser/ (rather than toolkit/) and therefore isn't being compiled on android, too?

Perhaps I misunderstood that bit, though.
(In reply to David Keeler [:keeler] (use needinfo?) from comment #89)
> (In reply to Wouter Verhelst :wouter from comment #87)
> > That only begs the question of "how". Does Firefox have a way to register
> > something in ways that cannot be (easily) altered by native installers?
> > After all, if we're trying to defend against a native installer writing a
> > malicious manifest file which breaks other people's modules, then we should
> > also try to defend against native installers trying to modify whatever
> > registry of "which add-on installed which module" we end up using, otherwise
> > what's the point.
> 
> I don't think we can defend against native installers here, but I don't
> think we're trying to, either.

I don't see what else we would be trying. Maybe I'm missing something?

> I think we're trying to prevent add-on A
> (installed from the web) from removing pkcs#11 entries added by add-on B
> (also installed from the web).

That is already covered by the fact that no add-on can touch a PKCS#11 module unless a Native Messaging Host JSON file was installed with "type": "pkcs11" and the add-on's ID in the "allowed_extensions" key.

Of course, the native code installer could allow both add-ons to meddle with the same PKCS#11 module, but then one might assume that the developers of the native code installer intended for multiple add-ons to be available or something.

Can you think of a scenario in which add-ons would be able to do "something bad" by meddling with another add-on's PKCS#11 module that they cannot do if we forbid it, given that they would have to be explicitly listed as allowed in the native messaging host JSON file anyway? Personally, I can't, but again, maybe I'm just missing something; and while I agree that dropping unneeded permissions is generally a good thing, it would have to be done in a way that can't easily be circumvented by the native code installation, otherwise what's the point -- and I can't think of such a way.

> > > > It might be that Firefox forbids the removal of Firefox-internal PKCS#11
> > > > modules, in which case your concern is covered.
> > > 
> > > Removing the internal module will fail in a safe way (you can try it
> > > yourself in the device manager). The larger concern is removing the built-in
> > > CA list. We should ensure that add-ons can't do this (see also bug 1391062).
> > 
> > Right. I think in the long run the XPCOM code should filter this out, but in
> > the next patch I'll explicitly filter out the nssckbi module in the
> > WebExtensions code.
> 
> Sounds good - add a test?

Done, and done. New version pushed.

Beyond the registration issue above, I think we're ready with this now.
Flags: needinfo?(tomica)
Flags: needinfo?(dkeeler)
(In reply to Wouter Verhelst :wouter from comment #91)
> Can you think of a scenario in which add-ons would be able to do "something
> bad" by meddling with another add-on's PKCS#11 module that they cannot do if
> we forbid it, given that they would have to be explicitly listed as allowed
> in the native messaging host JSON file anyway? Personally, I can't, but
> again, maybe I'm just missing something; and while I agree that dropping
> unneeded permissions is generally a good thing, it would have to be done in
> a way that can't easily be circumvented by the native code installation,
> otherwise what's the point -- and I can't think of such a way.

Oh - I think I misunderstood how this all works. If a native installer has to first list an add-on as being able to interact with a particular PKCS#11 module before it can add/remove it to/from Firefox, I think we're fine as-is.
Flags: needinfo?(dkeeler)
This looks like reasonable scope and API.
Flags: needinfo?(dveditz)
(In reply to David Keeler [:keeler] (use needinfo?) from comment #93)
> Oh - I think I misunderstood how this all works. If a native installer has
> to first list an add-on as being able to interact with a particular PKCS#11
> module before it can add/remove it to/from Firefox, I think we're fine as-is.

It's actually a little looser than that:
 - a native InstallerA would install a ModuleA and ExtensionA that could load/unload it, but 
 - a (malicious) native InstallerB could craft a manifest that would give ExtensionB access to load/unload ModuleA.

Though as I said above, I think we are not guarding from malicious native apps, as those could do whatever, even modify the NSS database directly.
Flags: needinfo?(tomica)
Attachment #8886230 - Flags: review+ → review?(tomica)
Comment on attachment 8886230 [details]
Bug 1357391 - Implement a PKCS#11 management API

https://reviewboard.mozilla.org/r/157012/#review187134

This looks good, thanks.  

In bug 1386427, I extracted the common logic into NativeManifests.jsm, so please switch to that.  (I asked for expedited review from Kris for parts 1-3, so you can now rebase your work on top of those patches if you want to speed this up to increase the chances of shipping in 57).

r=me, with the issues addressed, and ask :kmag for final review.

::: browser/components/extensions/ext-pkcs11.js:6
(Diff revision 9)
> +"use strict";
> +
> +XPCOMUtils.defineLazyModuleGetters(this,
> +  {
> +    HostManifestManager: "resource://gre/modules/NativeMessaging.jsm",
> +    ctypes: "resource://gre/modules/ctypes.jsm",

nit: sort imports alphabetically

::: browser/components/extensions/ext-pkcs11.js:18
(Diff revision 9)
> +  "nsIPKCS11ModuleDB"
> +);
> +
> +const findModuleByPath = function(path) {
> +  let modules = pkcs11db.listModules();
> +  while (modules.hasMoreElements()) {

nit: you can use the IterSimpleEnumerator [1] helper here.

1) http://searchfox.org/mozilla-central/search?q=IterSimpleEnumerator

::: browser/components/extensions/ext-pkcs11.js:36
(Diff revision 9)
> +      if (!manifestCache.has(name)) {
> +        let hostInfo = await HostManifestManager.readManifest(name, context, "pkcs11");
> +        if (hostInfo && !hostInfo.manifest.path.includes(ctypes.libraryName("nssckbi"))) {
> +          manifestCache.set(name, hostInfo.manifest);
> +        } else {
> +          return Promise.reject({message: `This extension does not have permission to use native PKCS#11 module ${name} (or the module is not installed)`});

In the new `NativeManifests.jsm` implementation, I added warnings to the console when there are manifest issues (including permissions), so now the rejection message can simply say "No such PKCS#11 module $name".

::: browser/components/extensions/ext-pkcs11.js:78
(Diff revision 9)
> +          let manifest = await getManifest(name);
> +          if (!manifest.description) {
> +            return Promise.reject({message: `The description field in the manifest for PKCS#11 module ${name} must have a value`});
> +          }
> +          try {
> +            pkcs11db.addModule(manifest.description, manifest.path, flags, 0);

I missed this earlier, but apparently we support relative paths in the native messaging manifest (relative to the manifest location), at least on Windows.  Seems easy to keep this consistent for pkcs11 modules, so please add that logic (the `lookupApplication/lookupManifest` method returns an object with the `.path` of the manifest that you can use).

http://searchfox.org/mozilla-central/source/toolkit/components/extensions/NativeMessaging.jsm#186-190

::: browser/components/extensions/test/xpcshell/test_ext_pkcs11_management.js:3
(Diff revision 9)
> +"use strict";
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "MockRegistry", "resource://testing-common/MockRegistry.jsm");

nit: you can use `defineModuleGetters` here as well

::: browser/components/extensions/test/xpcshell/test_ext_pkcs11_management.js:10
(Diff revision 9)
> +XPCOMUtils.defineLazyModuleGetter(this, "setTimeout", "resource://gre/modules/Timer.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "ctypes", "resource://gre/modules/ctypes.jsm");
> +
> +do_get_profile();
> +let tmpDir = FileUtils.getDir("TmpD", ["PKCS11"]);
> +tmpDir.createUnique(Ci.nsIFile.DIRECTORY_TYPE, FileUtils.PERMS_DIRECTORY);

After bug 1386427, the directory service points to the parent "native manifests" directory, with a separate dir for each type, so you need to also create the `PKCS11Modules`/`pkcs11-modules` subdir, depending on the platform (see patches in that bug for examples).

::: browser/components/extensions/test/xpcshell/test_ext_pkcs11_management.js:44
(Diff revision 9)
> +  switch (AppConstants.platform) {
> +    case "macosx":
> +    case "linux":
> +      let dirProvider = {
> +        getFile(property) {
> +          if (property == "XREUserNativeMessaging") {

This is now called `XRE*NativeManifests` (and points to the parent dir).

::: browser/components/extensions/test/xpcshell/test_ext_pkcs11_management.js:64
(Diff revision 9)
> +        await writeManifest(module);
> +      }
> +      break;
> +
> +    case "win":
> +      const REGKEY = String.raw`Software\Mozilla\NativeMessagingHosts`;

This is now under `Software\\Mozilla\\PKCS11Modules`

::: browser/components/extensions/test/xpcshell/test_ext_pkcs11_management.js:85
(Diff revision 9)
> +  }
> +}
> +
> +add_task(async function test_install_uninstall() {
> +  function background() {
> +    browser.pkcs11.isModuleInstalled("testmodule").then(result => {

nti: the background method can be `async` as well, so just do that and switch these to `await`s.

::: browser/components/extensions/test/xpcshell/test_ext_pkcs11_management.js:113
(Diff revision 9)
> +    }).then(() => {
> +      return browser.pkcs11.isModuleInstalled("testmodule");
> +    }).then(result => {
> +      return browser.test.assertFalse(result, "PKCS#11 module is no longer installed after we uninstall it");
> +    }).then(() => {
> +      return browser.pkcs11.isModuleInstalled("othermodule").catch(invalidModuleError => {

nit: `browser.test.assertRejects(...`

::: browser/components/extensions/test/xpcshell/test_ext_pkcs11_management.js:138
(Diff revision 9)
> +
> +  await setupManifests([
> +    {
> +      name: "testmodule",
> +      description: "PKCS#11 Test Module",
> +      path: "../../../../../security/manager/ssl/tests/unit/pkcs11testmodule/" + ctypes.libraryName("pkcs11testmodule"),

This is a big ugly but probably unavoidable without jumping throw a few hoops.  Please at least define base dir as the const at the top.

::: browser/components/extensions/test/xpcshell/test_ext_pkcs11_management.js:160
(Diff revision 9)
> +  let extension = ExtensionTestUtils.loadExtension({
> +    manifest: {
> +      permissions: ["pkcs11"],
> +      applications: {"gecko": {id: "pkcs11@tests.mozilla.org"}},
> +    },
> +    background: `(${background})()`,

nit, `background: background,` is enough, or simply  `background,`.

::: toolkit/components/extensions/NativeMessaging.jsm:156
(Diff revision 9)
>        throw new Error(`Invalid application "${application}"`);
>      }
>      return this.init().then(() => this._lookup(application, context));
>    },
> +
> +  readManifest(application, context, type) {

my changes in bug 1386427 now include this check, so please undo this and switch to using `NativeManifests.jsm`.
Attachment #8886230 - Flags: review?(tomica) → review+
Comment on attachment 8886230 [details]
Bug 1357391 - Implement a PKCS#11 management API

https://reviewboard.mozilla.org/r/157012/#review181542

> Honestly, I'm not entirely sure.
> 
> The XPCOM method requires it. If you don't specify any value, adding our particular module will fail. Traditionally, we've been specifying 0x1 << 28, which is PKCS11_PUB_READABLE_CERT_FLAG, and which states that certificates can be read from the card without being logged in.
> 
> I'm sure other PKCS#11 modules might need other values.
> 
> Having said all that, we could default it to being zero, I suppose.

flags parameter now defaults to zero

> This can could be easier to read as an async method.

this function will be dropped once the code for 1386427 lands, so no longer relevant (and dropping the issue)
(In reply to Wouter Verhelst :wouter from comment #97)
> Comment on attachment 8886230 [details]
> Bug 1357391: Implement a PKCS#11 management API
> 
> https://reviewboard.mozilla.org/r/157012/#review181542
> 
> > Honestly, I'm not entirely sure.
> > 
> > The XPCOM method requires it. If you don't specify any value, adding our particular module will fail. Traditionally, we've been specifying 0x1 << 28, which is PKCS11_PUB_READABLE_CERT_FLAG, and which states that certificates can be read from the card without being logged in.
> > 
> > I'm sure other PKCS#11 modules might need other values.
> > 
> > Having said all that, we could default it to being zero, I suppose.
> 
> flags parameter now defaults to zero

Most of the tokens I know certificate is not protected.
Estonian cards have 2 keypairs with public certificates (Auth and Signature) and if you try login somewhere
It ask both pin-s and uses only auth certificate.

This flag makes sure, that Certificate is selected before asking PIN.

I hope new default will use later behaviour
Attachment #8886230 - Flags: review+ → review?(tomica)
There's a flag that can be set on the module that says the certs can be read without logging in (PUBLICLY READABLE CERTS). Unfortunately you can't set it from the firefox UI, but if you have and installer, or use manually use modutil, you can set the flag.

If you set the flag and NSS can't publicly read the certs (or there isn't a corresponding public key to the private key, which NSS needs to tell if the cert is a user cert), then your token certs will just not be visible until you explicitly log into your token.

bob
Apparently I am now officially too late to get this into FF 57 anymore.

I wish someone had told me that branch day was so close, or I would have spent way more time yesterday trying to get this done. I think it's completely ready for merging now, it's just 24 hours too late :-(

According to https://wiki.mozilla.org/Release_Management/Uplift_rules, if I want to even be considered for an exception and be uplifted into 57, I need to be in mozilla-central first. Can that at least be expedited now? That way, even if I'm not granted an exception, at least we'll still be in 58.
Hi there,

As for us we really need this API to be shipped along with FF 57.
I ve tested it extensively with an prototype extension and it suits our needs.

We can't afford waiting 6-8 weeks for the next update with user online services broken.

Isn't possible that it could be landed for an 57 update (say 57.0.1) ?

Thanks.
On IRC, earlier:

> 18:18 < wouter> zombie: should I needinfo kmag then? or is that not the right way to do it?
> 18:20 <@zombie> you can, but don't need to.  flag me to review the tests, and then him for final review anyway

Did that, you reviewed it, I fixed the nits (and pushed) yesterday, so asking for final review now.
Attachment #8886230 - Flags: review?(kmaglione+bmo)
Hi all,


At first, thanks a lot for all your work specially for Wouter Verhelst and Firefox Team.

To remember, I work on ASIP Santé a governement French agency which is responsible of French Professionnal Health Smartcard, i.e. CPS.

Today, we have deployed 1,500,000 smartcards. For example, after Firefox 55 release, we have more than 80,000 users who automatically update our XPI Extension (Extension CPS).

If it is not on Firefox 57, it could be a big issue for our Firefox users.

We are ready to support and help you to verify the good integration in a 57 beta.

Thanks again,


Regards
Comment on attachment 8886230 [details]
Bug 1357391 - Implement a PKCS#11 management API

https://reviewboard.mozilla.org/r/157012/#review188178

Thanks, r=me (though you don't need to ask for review again after you get "r+ with nits").

::: browser/components/extensions/ext-pkcs11.js:76
(Diff revision 10)
> +          * nsIPKCS11ModuleDB.addModule method
> +          * @returns {Promise} When the Promise resolves, the module will have
> +          * been installed. When it is rejected, the module either is already
> +          * installed or could not be installed for some reason.
> +          */
> +        async installModule(name, flags = 0) {

nit: please define the default value in the json schema instead.

::: browser/components/extensions/test/xpcshell/test_ext_pkcs11_management.js:8
(Diff revision 10)
> +XPCOMUtils.defineLazyModuleGetters(this,
> +  {
> +    ctypes: "resource://gre/modules/ctypes.jsm",
> +    MockRegistry: "resource://testing-common/MockRegistry.jsm",
> +    OS: "resource://gre/modules/osfile.jsm",
> +    setTimeout: "resource://gre/modules/Timer.jsm",

This is unused.
Attachment #8886230 - Flags: review?(tomica) → review+
(In reply to Raul Metsma from comment #98)
> > flags parameter now defaults to zero
> 
> Most of the tokens I know certificate is not protected.
> Estonian cards have 2 keypairs with public certificates (Auth and Signature)
> and if you try login somewhere
> It ask both pin-s and uses only auth certificate.
> 
> This flag makes sure, that Certificate is selected before asking PIN.
> 
> I hope new default will use later behaviour

I'm not sure I understand this comment.  Wouter, can you please explain/translate for non-pkcs developers?


(In reply to Wouter Verhelst :wouter from comment #101)
> Apparently I am now officially too late to get this into FF 57 anymore.
> 
> I wish someone had told me that branch day was so close

Release calendar is public and (usually) stable months in advance: 
https://wiki.mozilla.org/RapidRelease/Calendar

I did suggest you rebase on top of my not-yet-landed patches to speed things up (and even bugged Kris to bump those parts ahead in his queue, ahead of other P1 bugs).


> According to https://wiki.mozilla.org/Release_Management/Uplift_rules, if I
> want to even be considered for an exception and be uplifted into 57, I need
> to be in mozilla-central first. Can that at least be expedited now? That
> way, even if I'm not granted an exception, at least we'll still be in 58.

Yes, getting the review from :kmag and landing on central is the next step, weather you get uplift approved or not.

And for the later approval request, my assessment is:

  1) Shipping this in 57 would be beneficial to cover existing functionality we are losing with legacy extensions.
  2) From the code point of view, I consider this patch to be fairly low risk, as it adds a new API without touching any other part of our codebase, which significantly limits the risk of regressions.
(In reply to Tomislav Jovanovic :zombie from comment #106)
> (In reply to Raul Metsma from comment #98)
> > > flags parameter now defaults to zero
> > 
> > Most of the tokens I know certificate is not protected.
> > Estonian cards have 2 keypairs with public certificates (Auth and Signature)
> > and if you try login somewhere
> > It ask both pin-s and uses only auth certificate.
> > 
> > This flag makes sure, that Certificate is selected before asking PIN.
> > 
> > I hope new default will use later behaviour
> 
> I'm not sure I understand this comment.  Wouter, can you please
> explain/translate for non-pkcs developers?

The default value for the "flags" parameter to installModule that I
specified (0) does not specify the "certificates are public" flag (which
is the 1<<28 that I mentioned earlier).

One thing that PKCS#11 modules must do is to offer applications using
the PKCS#11 API to access its services the ability to read certificates
from the token (through the C_FindObjects* set of functions). Something
else they need to do is to offer a "logon" method, allowing the user to
log on to the token. The standard does not mention whether a user needs
to be logged on to the token in order to be allowed to read a
certificate, however; this is because the PKCS#11 standard is a generic
standard that was not written *just* for authentication smartcards, and
there can be use cases where reading data (such as certificates) from
the token must not be allowed unless the user has first logged on. There
is an error code that a PKCS#11 module can return when an application
tries to read data that it is not allowed to; however, Firefox deals
with the issue by way of that "public certificate" flag instead: if the
flag was not specified when the token was installed, then Firefox will
require that the user logs on to the token before allowing the selection
of an authentication certificate. If it was, then Firefox will simply
try to read certificates as-is.

It is unfortunate that the default in Firefox (i.e., when no flags are
selected) is to require logging on to the token before a certificate can
be selected; most smartcards meant for authentication do not require it.
For historical reasons though, I suppose it's hard to change that
default now. I did consider choosing a default of 1<<28 for this option.
In the end I decided against it, because I thought it might be
confusing. However, perhaps now is indeed the best time to think about
this.

Perhaps someone of the NSS developers could chime in and give an opinion
here?

> (In reply to Wouter Verhelst :wouter from comment #101)
> > Apparently I am now officially too late to get this into FF 57 anymore.
> > 
> > I wish someone had told me that branch day was so close
> 
> Release calendar is public and (usually) stable months in advance: 
> https://wiki.mozilla.org/RapidRelease/Calendar

Yes, in hindsight it's obvious that would have been the case, but I
wasn't aware of it. If I had been, obviously I would have tried to get
things done earlier.

It's especially frustrating as that day was a pretty slow day for me, so
I *could* have gotten things done in time, had I known. Too late now, I
suppose.

> I did suggest you rebase on top of my not-yet-landed patches to speed things
> up (and even bugged Kris to bump those parts ahead in his queue, ahead of
> other P1 bugs).

Indeed; however, it wasn't clear to me why that was. For future
reference, if you ever do something like that again in a future
mentoring situation, I suggest you mention somewhat more explicitly that
merge day is so close by, and that that is why you're trying to speed
things up -- it just wasn't clear to me.

> > According to https://wiki.mozilla.org/Release_Management/Uplift_rules, if I
> > want to even be considered for an exception and be uplifted into 57, I need
> > to be in mozilla-central first. Can that at least be expedited now? That
> > way, even if I'm not granted an exception, at least we'll still be in 58.
> 
> Yes, getting the review from :kmag and landing on central is the next step,
> weather you get uplift approved or not.
> 
> And for the later approval request, my assessment is:
> 
>   1) Shipping this in 57 would be beneficial to cover existing functionality
> we are losing with legacy extensions.
>   2) From the code point of view, I consider this patch to be fairly low
> risk, as it adds a new API without touching any other part of our codebase,
> which significantly limits the risk of regressions.

Thanks for that.

I'll add that not merging would result in major usability issues (as I
explain in large detail in comment 17) for a relatively small, but still
significant number of users. This bug mentions:

- All people of Belgian nationality or who work in Belgium, who use
  Firefox, and who need to authenticate to government websites (e.g., to
  declare their taxes)
- A similar number of people in Estonia;
- Any one of the 1.5 million health care professionals in France who
  need to deal with the French health administration and wish to do so
  using Firefox.

I'm sure there are more, but these are the people who spoke up in the
bug report.
Keywords: checkin-needed
This still needs review from Kris.
Keywords: checkin-needed
Comment on attachment 8886230 [details]
Bug 1357391 - Implement a PKCS#11 management API

https://reviewboard.mozilla.org/r/157012/#review188914

Thanks. This is close, but there are some nits to fix.

::: browser/components/extensions/ext-pkcs11.js:17
(Diff revision 10)
> +const findModuleByPath = function(path) {
> +  let modules = pkcs11db.listModules();
> +  for (let module of XPCOMUtils.IterSimpleEnumerator(modules, Ci.nsIPKCS11Module)) {
> +    if (module && module.libName === path) {
> +      return module;
> +    }
> +  }
> +  return null;
> +};

Is there a particular reason we need to implement a custom iterator to find the module by path rather than just using `findModuleByName`? Iterators like this in JS are extremely expensive.

::: browser/components/extensions/ext-pkcs11.js:29
(Diff revision 10)
> +    let manifestCache = new Map();
> +    let getManifest = async function(name) {
> +      if (!manifestCache.has(name)) {

Please use a DefaultMap for this.

::: browser/components/extensions/ext-pkcs11.js:32
(Diff revision 10)
> +this.pkcs11 = class extends ExtensionAPI {
> +  getAPI(context) {
> +    let manifestCache = new Map();
> +    let getManifest = async function(name) {
> +      if (!manifestCache.has(name)) {
> +        let hostInfo = await NativeManifests.lookupManifest("pkcs11", name, context);

With the current version of this code, multiple calls for the same module name before the first call has resolved will result in multiple lookups. That wouldn't be a problem if you used a DefaultMap.

::: browser/components/extensions/ext-pkcs11.js:33
(Diff revision 10)
> +  getAPI(context) {
> +    let manifestCache = new Map();
> +    let getManifest = async function(name) {
> +      if (!manifestCache.has(name)) {
> +        let hostInfo = await NativeManifests.lookupManifest("pkcs11", name, context);
> +        if (hostInfo && !hostInfo.manifest.path.includes(ctypes.libraryName("nssckbi"))) {

This should be `.endsWith`. Or, really, it should use the OS.Path API to get the actual leaf name. And on Windows and OS-X, the comparison needs to be case insensitive.

::: browser/components/extensions/ext-pkcs11.js:51
(Diff revision 10)
> +      pkcs11: {
> +        /**
> +          * Verify whether a given PKCS#11 module is installed.
> +          *
> +          * @param {string} name The name of the module, as specified in
> +          * the manifest file.

Nit: Please indent multi-line descriptions so that subsequent lines align with the start of the description on previous lines. And I generally prefer the description to start on the next line from the `@param` tag, aligned with the opening `{`

::: browser/components/extensions/ext-pkcs11.js:60
(Diff revision 10)
> +          } catch (e) {
> +            Cu.reportError(e);

Is there a reason for the try-catch here?

::: browser/components/extensions/ext-pkcs11.js:70
(Diff revision 10)
> +        /**
> +          * Install a PKCS#11 module
> +          *
> +          * @param {string} name The name of the module, as specified in
> +          * the manifest file.
> +          * @param {integer} flags Any flags to be passed on to the

Nit: `@param {integer] [flags = 0]`

::: browser/components/extensions/ext-pkcs11.js:83
(Diff revision 10)
> +          } catch (e) {
> +            Cu.reportError(e);

Unless there's a particular exception you're expectng here, you should just let exceptions pass through and be reported to the extension as unexpected errors. Same below.

::: browser/components/extensions/ext-pkcs11.js:138
(Diff revision 10)
> +            while (slots.hasMoreElements()) {
> +              let slot = slots.getNext().QueryInterface(Ci.nsIPKCS11Slot);

Nit: `for (let slot of XPCOMUtils.IterSimpleEnumerator(module.listSlots(), Ci.nsIPKCS11Slot))`

::: browser/components/extensions/ext-pkcs11.js:140
(Diff revision 10)
> +            }
> +            let rv = [];
> +            let slots = module.listSlots();
> +            while (slots.hasMoreElements()) {
> +              let slot = slots.getNext().QueryInterface(Ci.nsIPKCS11Slot);
> +              let token = slot.getToken().QueryInterface(Ci.nsIPK11Token);

Nit: No need for the QueryInterface.

::: browser/components/extensions/ext-pkcs11.js:141
(Diff revision 10)
> +              let slotobj = {};
> +              slotobj.name = slot.name;
> +              slotobj.token = null;

Please define these properties as part of the object literal.

::: browser/components/extensions/ext-pkcs11.js:146
(Diff revision 10)
> +                slotobj.token.name = token.tokenName;
> +                slotobj.token.manufacturer = token.tokenManID;
> +                slotobj.token.HWVersion = token.tokenHWVersion;
> +                slotobj.token.FWVersion = token.tokenFWVersion;
> +                slotobj.token.serial = token.tokenSerialNumber;
> +                slotobj.token.isLoggedIn = token.isLoggedIn();

Please define these properties as part of the object literal.

::: browser/components/extensions/test/xpcshell/test_ext_pkcs11_management.js:101
(Diff revision 10)
> +      browser.test.assertEq(slots[0].name, "Test PKCS11 Slot", "The first slot name matches the expected name");
> +      browser.test.assertFalse(slots[0].token, "The first slot has no token");
> +      browser.test.assertEq(slots[1].name, "Test PKCS11 Slot 二", "The second slot name matches the expected name");
> +      browser.test.assertTrue(slots[1].token, "The second slot has a token");
> +      browser.test.assertEq(slots[1].token.name, "Test PKCS11 Tokeñ 2 Label", "The token name matches the expected name");
> +      browser.test.assertEq(slots[1].token.manufacturer, "Test PKCS11 Manufacturer ID", "The token manufacturer matches the expected manufacturer");
> +      browser.test.assertEq(slots[1].token.HWVersion, "0.0", "The token hardware version matches the expected version");
> +      browser.test.assertEq(slots[1].token.FWVersion, "0.0", "The token firmware version matches the expected version");
> +      browser.test.assertEq(slots[1].token.serial, "", "The token has no serial number");

Nit: The expected value should be the first argument to `assertEq`.

::: browser/components/extensions/test/xpcshell/test_ext_pkcs11_management.js:118
(Diff revision 10)
> +      browser.test.assertFalse(isInstalled, "PKCS#11 module is no longer installed after we uninstall it");
> +      await browser.pkcs11.installModule("testmodule");
> +      isInstalled = await browser.pkcs11.isModuleInstalled("testmodule");
> +      browser.test.assertTrue(isInstalled, "Installing the PKCS#11 module without flags parameter succeeds");
> +      await browser.pkcs11.uninstallModule("testmodule");
> +      await browser.test.assertRejects(browser.pkcs11.isModuleInstalled("nonexistingmodule"), /No such PKCS#11 module nonexistingmodule/,

Nit: Subsequent arguments need to be aligned with the first argument. Either move the first argument to a new line, or align the message string after the opening paren.

::: browser/components/extensions/test/xpcshell/test_ext_pkcs11_management.js:120
(Diff revision 10)
> +      isInstalled = await browser.pkcs11.isModuleInstalled("testmodule");
> +      browser.test.assertTrue(isInstalled, "Installing the PKCS#11 module without flags parameter succeeds");
> +      await browser.pkcs11.uninstallModule("testmodule");
> +      await browser.test.assertRejects(browser.pkcs11.isModuleInstalled("nonexistingmodule"), /No such PKCS#11 module nonexistingmodule/,
> +        "We cannot access modules if no JSON file exists"
> +      );

Nit: Please move to previous line.
Attachment #8886230 - Flags: review?(kmaglione+bmo)
Comment on attachment 8886230 [details]
Bug 1357391 - Implement a PKCS#11 management API

I'd like feedback from keeler on the PSM parts of this before I give a final r+
Attachment #8886230 - Flags: feedback?(dkeeler)
Comment on attachment 8886230 [details]
Bug 1357391 - Implement a PKCS#11 management API

https://reviewboard.mozilla.org/r/157012/#review188914

> Is there a particular reason we need to implement a custom iterator to find the module by path rather than just using `findModuleByName`? Iterators like this in JS are extremely expensive.

My thinking was that PKCS#11 module names are really just human-modifiable strings; there is no guarantee that they're the same for any given module. An add-on could change the module name for whatever reason, or the user could load it manually under another name, and then the whole API would fail (you can't configure the same module twice, so an addon trying to install the module "foo" that was installed with the name "the foo module" would fail to find it if it tries to install it as "the Foo module", and would not be able to install it either).

The file name of the PKCS#11 module, however, is fixed (if you use a different name, the .so or .dylib or .dll or whatever won't be found, and the module can't be loaded), so that seems like a better candidate for a unique name.

It would probably make sense to add a "findModuleByPath" method to the nsIPKCS11ModuleDB interface instead of doing it in JS, but I didn't want to convolute the patch too much.

> Is there a reason for the try-catch here?

Previous reviewers suggested that we should not leak internal exceptions to the addons, because that might allow malicious addons to enumerate installed modules.

> Unless there's a particular exception you're expectng here, you should just let exceptions pass through and be reported to the extension as unexpected errors. Same below.

This was also for the "we don't want to leak" reason. Happy to drop all that if you tell me to do so, but I thought I'd mention the reason first.

(your other comments are taken care of in my checkout, apart from handful that require more time than I have at 23:30)
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #110)
> Comment on attachment 8886230 [details]
> Bug 1357391: Implement a PKCS#11 management API
> 
> I'd like feedback from keeler on the PSM parts of this before I give a final
> r+

Just in case you missed it: keeler already commented on the bug, and gave r+ (see comment 88 and comment 93).
(In reply to Wouter Verhelst :wouter from comment #112)
> (In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're
> blocked) from comment #110)
> > Comment on attachment 8886230 [details]
> > Bug 1357391: Implement a PKCS#11 management API
> > 
> > I'd like feedback from keeler on the PSM parts of this before I give a final
> > r+
> 
> Just in case you missed it: keeler already commented on the bug, and gave r+
> (see comment 88 and comment 93).

Actually, not the r+ part, but the other part is true
Comment on attachment 8886230 [details]
Bug 1357391 - Implement a PKCS#11 management API

https://reviewboard.mozilla.org/r/157012/#review188982

Just to double-check, the code using the PSM API runs in the parent process, right? (I mean, it must, for the test to be passing...)
The uses of the PSM APIs look correct to me. I just have the one comment about the test.

::: browser/components/extensions/test/xpcshell/test_ext_pkcs11_management.js:102
(Diff revision 10)
> +      await browser.pkcs11.installModule("testmodule", 0);
> +      isInstalled = browser.pkcs11.isModuleInstalled("testmodule");
> +      browser.test.assertTrue(isInstalled, "PKCS#11 module is installed after we install it");
> +      let slots = await browser.pkcs11.getModuleSlots("testmodule");
> +      browser.test.assertEq(slots[0].name, "Test PKCS11 Slot", "The first slot name matches the expected name");
> +      browser.test.assertFalse(slots[0].token, "The first slot has no token");

I don't think this is sound - the test module simulates adding and removing the token in the 0th slot every 50ms or something, so this probably won't work every time.
(In reply to Wouter Verhelst :wouter from comment #111)
> > Is there a particular reason we need to implement a custom iterator to find the module by path rather than just using `findModuleByName`? Iterators like this in JS are extremely expensive.
> 
> My thinking was that PKCS#11 module names are really just human-modifiable
> strings; there is no guarantee that they're the same for any given module.
> An add-on could change the module name for whatever reason, or the user
> could load it manually under another name, and then the whole API would fail
> (you can't configure the same module twice, so an addon trying to install
> the module "foo" that was installed with the name "the foo module" would
> fail to find it if it tries to install it as "the Foo module", and would not
> be able to install it either).

The add-on can't change the module name at runtime, since it's encoded in the
manifest, and loaded modules aren't persisted across sessions, so that shouldn't
really be an issue.

In any case, this code probably shouldn't be run often enough for the performance
to an issue, so I don't have that strong a reason on it.

> > Is there a reason for the try-catch here?
> 
> Previous reviewers suggested that we should not leak internal exceptions to
> the addons, because that might allow malicious addons to enumerate installed
> modules.

The framework guarantees that internal exceptions won't be leaked to the add-on.

(In reply to Wouter Verhelst :wouter from comment #113)
> (In reply to Wouter Verhelst :wouter from comment #112)
> > Just in case you missed it: keeler already commented on the bug, and gave r+
> > (see comment 88 and comment 93).
> 
> Actually, not the r+ part, but the other part is true

That's fine, but I'd still rather have an explicit f+ :)

(In reply to David Keeler [:keeler] (use needinfo?) from comment #114)
> Just to double-check, the code using the PSM API runs in the parent process,
> right? (I mean, it must, for the test to be passing...)
> The uses of the PSM APIs look correct to me. I just have the one comment
> about the test.

Yes, it runs in the parent process.

Thanks
Comment on attachment 8886230 [details]
Bug 1357391 - Implement a PKCS#11 management API

https://reviewboard.mozilla.org/r/157012/#review188982

> I don't think this is sound - the test module simulates adding and removing the token in the 0th slot every 50ms or something, so this probably won't work every time.

I had noticed that, in the C_WaitForSlotEvent() call. I had been working under the assumption that that would not ever be called in the xpcshell, but giving it some more thought, I can see why that assumption might (or maybe should) be wrong, so if it works now it would essentially be a race.

I'll drop that particular assert for the next version.
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #115)
> The add-on can't change the module name at runtime, since it's encoded in the
> manifest, and loaded modules aren't persisted across sessions, so that

No, that's a misunderstanding -- they *are* persisted across sessions.
Attachment #8886230 - Flags: review?(kmaglione+bmo)
Attachment #8886230 - Flags: review?(kmaglione+bmo)
whoops, I messed up, sorry -- let me try that again
sorry for the panicked set of review pushes there -- please interdiff 10 to 13.
Attachment #8886230 - Flags: review?(kmaglione+bmo)
Comment on attachment 8886230 [details]
Bug 1357391 - Implement a PKCS#11 management API

https://reviewboard.mozilla.org/r/157012/#review189920

::: browser/components/extensions/ext-pkcs11.js:4
(Diff revision 13)
> +"use strict";
> +
> +XPCOMUtils.defineLazyModuleGetters(this,
> +  {

Nit: Please move opening brace to the previous line.

::: browser/components/extensions/ext-pkcs11.js:9
(Diff revision 13)
> +  {
> +    ctypes: "resource://gre/modules/ctypes.jsm",
> +    NativeManifests: "resource://gre/modules/NativeManifests.jsm",
> +    OS: "resource://gre/modules/osfile.jsm",
> +  }
> +);

Nit: Move to previous line.

::: browser/components/extensions/ext-pkcs11.js:15
(Diff revision 13)
> +
> +XPCOMUtils.defineLazyServiceGetter(this,
> +  "pkcs11db",
> +  "@mozilla.org/security/pkcs11moduledb;1",
> +  "nsIPKCS11ModuleDB"
> +);

Previous line.

::: browser/components/extensions/test/xpcshell/test_ext_pkcs11_management.js:4
(Diff revision 13)
> +"use strict";
> +
> +XPCOMUtils.defineLazyModuleGetters(this,
> +  {

Nit: Move to previous line.

::: browser/components/extensions/test/xpcshell/test_ext_pkcs11_management.js:9
(Diff revision 13)
> +  {
> +    ctypes: "resource://gre/modules/ctypes.jsm",
> +    MockRegistry: "resource://testing-common/MockRegistry.jsm",
> +    OS: "resource://gre/modules/osfile.jsm",
> +  }
> +);

Move to previous line.
Attachment #8886230 - Flags: review?(kmaglione+bmo) → review+
Hi all,

I bump into the following:

Use case: uninstalling pkcs11 module when addon is disabled.
------------------------------------------------------------

I use the following code :

// Installing handler
window.addEventListener('unload', function(e) { var test_pkcs11 = new PkcsManager(); test_pkcs11.unregisterModuleNotWait();}, false);

---------------------------
---------------------------

// Uninstall function in class PkcsManager
 this.unregisterModuleNotWait = function() {
    
	// check pkcs11 API is available ...
	if (typeof browser.pkcs11 !== 'undefined') {
	  console.log("uninstall module not wait !!");
	  var gotResult = browser.pkcs11.uninstallModule("unknown");
	  // console.log("gotResult: ", gotResult);
	  gotResult.then(() => {console.log("OK");}).catch(err => console.error(err));
	}
	else {
	  console.log("pkcs11 API is not available yet (unregisterModuleNotWait).");
	}
  }

I debug our addon with 'about:debugging' Firefox feature.

The only way for 'unload' event to be fired, is to reload the extension when clicking the 'Reload' link.
When I click 'Reload' the unregisterModuleNotWait() is well called (fastly! because I can hardly see the 'uninstall module not wait !!' message in console window).

But this is where the problem is, the pkcs11.uninstallModule() seems to do nothing:
 - no OK status logged => then() OR no error logged => catch(), the promise seems to be pending ...
 - the pkcs11 module is still present in 'Security Devices' pane

Has anybody already tested this case ?

Thanks.

Note: For my point of view this is a problem liked to new WebExtension restartless design.
Sorry, I forgot to mention that if I pass a known installed module:

var gotResult = browser.pkcs11.uninstallModule("known_pkcs11");

I meet the same behaviour.

Regards.
Sorry, I don't understand what to do with the two jobs/tasks? displayed in the dashboard.

Could you tell me more.
all try builds were successful, except for the Windows ones where failure to sign the build resulted in the xpcshell tests not being started. This happened on the previous run too[1], but then kmag asked for another try run[2] and then that one did work.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2aba21a932561a856390d2db3b2f40917cc161a
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d971db4996a

I'm guessing that I don't have enough permissions or something. The changes since the previous try run are limited; I don't actually think there might have been anything that could cause problems, but I thought I'd just do another try run to be on the safe side. It seems to still work on OSX and Linux, so I think it's safe to assume all will be fine.

Please check in.
Keywords: checkin-needed
(In reply to BPER_ILEX from comment #124)
> But this is where the problem is, the pkcs11.uninstallModule() seems to do
> nothing:
>  - no OK status logged => then() OR no error logged => catch(), the promise
> seems to be pending ...
>  - the pkcs11 module is still present in 'Security Devices' pane

You start an async function from the unload event; I'm not sure that that is actually supported. Unloading the module at a different point in time does work though.

It's a good point that it should be possible, however, but I don't think it's necessary to have that in this bug. Please file a separate issue about that.

(In reply to BPER_ILEX from comment #129)
> Sorry, I don't understand what to do with the two jobs/tasks? displayed in
> the dashboard.
> 
> Could you tell me more.

That's just internal cookery to ensure that things compile before they get merged into the mozilla-central repository. Nothing for you to worry about ;-)
(In reply to David Keeler [:keeler] (use needinfo?) from comment #114)
> > +      browser.test.assertFalse(slots[0].token, "The first slot has no token");
> 
> I don't think this is sound - the test module simulates adding and removing
> the token in the 0th slot every 50ms or something, so this probably won't
> work every time.

I dropped that assert, but it does mean that the case of "slot with no token found" is not being tested for. Should I perhaps file another bug to update the PKCS#11 test module to also add an empty third slot?
Flags: needinfo?(dkeeler)
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/82920c8bb33a
Implement a PKCS#11 management API r=kmag,zombie
Keywords: checkin-needed
(In reply to Wouter Verhelst :wouter from comment #132)
> (In reply to David Keeler [:keeler] (use needinfo?) from comment #114)
> > > +      browser.test.assertFalse(slots[0].token, "The first slot has no token");
> > 
> > I don't think this is sound - the test module simulates adding and removing
> > the token in the 0th slot every 50ms or something, so this probably won't
> > work every time.
> 
> I dropped that assert, but it does mean that the case of "slot with no token
> found" is not being tested for. Should I perhaps file another bug to update
> the PKCS#11 test module to also add an empty third slot?

Sure - thanks! (Core :: Security: PSM would be the best component)
Flags: needinfo?(dkeeler)
Filed as bug 1404421
It looks like you need to fix the path separator characters in the `modulepath` global.
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #137)
> It looks like you need to fix the path separator characters in the
> `modulepath` global.

Unfortunately, it was not that simple :-)

The problem was that Windows is special-cased in the NativeManifests.jsm code: if a relative path is provided, then the absolute path of the Native manifest is prepended. The same does not happen on Linux or OSX.

Since I use a relative path to the test PKCS#11 module, that meant that on Windows the relative path resolution used the wrong start point, and therefore the module could not be found when we try to actually install it.

Fixed it by using OS.File.getCurrentDirectory() inside the xpcshell-test to create a PKCS#11 native manifest that has an absolute path rather than a relative one.

While at it, I also removed a sentence from the commit message that was a leftover statement from before NativeManifests.jsm existed, but is no longer accurate now.

Tested on Windows and Linux, works for both.

Please check in again.
Flags: needinfo?(w)
Keywords: checkin-needed
Depends on: 1404624
Depends on: 1404632
Depends on: 1404421
Pushed by tomica@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/22374473d24f
Implement a PKCS#11 management API r=kmag,zombie
Keywords: checkin-needed
Blocks: 1404632
No longer depends on: 1404632
Blocks: 1404624
No longer depends on: 1404624
https://hg.mozilla.org/mozilla-central/rev/22374473d24f
Status: NEW → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Comment on attachment 8886230 [details]
Bug 1357391 - Implement a PKCS#11 management API

Note: I'm not 100% sure whether this is the right uplift target. I would like to see it be shipped with ff 57. If not, please redirect to the right place :) thanks.

Approval Request Comment
[Feature/Bug causing the regression]: Drop of support for legacy add-ons in ff57
[User impact if declined]: Users will have to manually configure PKCS#11 modules. See comment 17 for a more in-depth explanation and rationale.
[Is this code covered by automated tests?]: yes, xpcshell-test added as part of this changeset.
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low risk
[Why is the change risky/not risky?]: adds a new API without touching any other part of the codebase.
[String changes made/needed]: only one, for the additional permission. This was approved by Scott in comment 70.
Attachment #8886230 - Flags: approval-mozilla-beta?
Flod, what do you think about the new string? 
(Hello Wouter, nice to see you here ;)
Flags: needinfo?(francesco.lodolo)
I think the strings are the smaller concern on this path, I'm OK with uplift to Beta.

Note that this puts the /mobile permissions once again out of sync.
Flags: needinfo?(francesco.lodolo)
Hi Kev, as discussed on slack, could you/AndyM help determine if this is a must for 57 or not? We'd rather not take risky patches in 57 that might jeopardize 57 go live and ship quality. Thanks!
status-firefox57: --- → affected
Flags: needinfo?(kev)
I don't think this patch is particularly risky by itself. Unless there are add-ons installed that are using the API, it shouldn't have any outside effects.

It could be potentially risky when add-ons start using it, but loading PKCS#11 modules is risky on its own, and people are going to wind up doing that anyway.

The risk of not taking this in 57 is that it will no longer be possible for users to install PKCS#11 modules without a complicated, manual process, and that may cause them to switch to other browsers.
(Just a Nightly user/community member.)
Couldn't you just introduce a pref with a whitelist of addons that are (reviewed and) permitted to use this api?
(media.getusermedia.screensharing.allowed_domains does this on a per-domain basis.)
(In reply to Jan Andre Ikenmeyer [:darkspirit] from comment #147)
> (Just a Nightly user/community member.)
> Couldn't you just introduce a pref with a whitelist of addons that are
> (reviewed and) permitted to use this api?

No, reviewing imposes a burden upon the community for ever. An API is either safe, or not.
I do not believe this should be uplifted to beta, its a couple of weeks into the beta cycle and we shouldn't be uplifting new features at this stage. I don't imagine that people setup configuration for PKCS very often. Any configuration done in 56 will remain and waiting 6 weeks for the 58 release doesn't seem unreasonable.
(In reply to Andy McKay [:andym] from comment #149)
> I do not believe this should be uplifted to beta, its a couple of weeks into
> the beta cycle and we shouldn't be uplifting new features at this stage. I
> don't imagine that people setup configuration for PKCS very often. Any
> configuration done in 56 will remain and waiting 6 weeks for the 58 release
> doesn't seem unreasonable.

I think the main snag there is that we're running major campaigns to get people to switch to 57, and if one of the first things they have to do is go through some painful process to configure their auth devices, I doubt they'll be very happy.
(In reply to Ritu Kothari (:ritu) from comment #145)
> Hi Kev, as discussed on slack, could you/AndyM help determine if this is a
> must for 57 or not? We'd rather not take risky patches in 57 that might
> jeopardize 57 go live and ship quality. Thanks!

This is certainly not "a must", but "a very nice to have" and also "very low risk" at the same time.  Not sure if we have a policy for that category, or if 57 and legacy addon removal should make this a special exception.


(In reply to Andy McKay [:andym] from comment #149)
> I do not believe this should be uplifted to beta, its a couple of weeks into
> the beta cycle and we shouldn't be uplifting new features at this stage. I
> don't imagine that people setup configuration for PKCS very often. Any
> configuration done in 56 will remain and waiting 6 weeks for the 58 release
> doesn't seem unreasonable.

Is "no new features at this stage" an existing policy?  Also, I though uplifts are only strictly restricted from October 10th.
(In reply to Tomislav Jovanovic :zombie from comment #151)
> Is "no new features at this stage" an existing policy?  Also, I though
> uplifts are only strictly restricted from October 10th.

There was a request from Release Management to land new features before the soft freeze of Nightly, which was about 3 weeks. They have been kind enough to land features after that deadline for us. 

I'm not going to worry too much either way: it would be nice to land but I know we've all got a lot of other stuff landing and there's enough risk in 57 already. It's not a reflection on this patch or the feature and more an attempt to reduce the risk in 57 by only worrying about high priority patches.
(In reply to Andy McKay [:andym] from comment #152)
> (In reply to Tomislav Jovanovic :zombie from comment #151)
> > Is "no new features at this stage" an existing policy?  Also, I though
> > uplifts are only strictly restricted from October 10th.
> 
> There was a request from Release Management to land new features before the
> soft freeze of Nightly, which was about 3 weeks. They have been kind enough
> to land features after that deadline for us. 

Not sure whether you mean "three weeks ago" or "a period of three weeks a while back" here, but if the former, I think it's actually two weeks.

> I'm not going to worry too much either way: it would be nice to land but I
> know we've all got a lot of other stuff landing and there's enough risk in
> 57 already. It's not a reflection on this patch or the feature and more an
> attempt to reduce the risk in 57 by only worrying about high priority
> patches.

I realize (and probably agree) that from a general browser POV this API would be low priority. However, for the minority of cases where it's used, the lack of this API would be a nightmare scenario, even if only for six weeks.

This is particularly irksome as I know that our support people have far far far less issues getting our users to have a working authentication configuration with current Firefox, to the extent that in some cases when a user is unable to get things working, the standard reply is "please try again with Firefox". Losing that ability (or having to tell people things like "please try Firefox ESR and then update to 57" or similar madness) would be unfortunate.

Thanks for considering,
Summary: a way to sideload PKCS#11 modules → Allow an extension to configure Firefox security devices
(In reply to Jan Andre Ikenmeyer [:darkspirit] from comment #147)
> (Just a Nightly user/community member.)
> Couldn't you just introduce a pref with a whitelist of addons that are
> (reviewed and) permitted to use this api?
> (media.getusermedia.screensharing.allowed_domains does this on a per-domain
> basis.)

To add to Andy's point, this is no longer the case since bug 1315737 for similar reasons.
Comment on attachment 8886230 [details]
Bug 1357391 - Implement a PKCS#11 management API

Thanks Andy for your assessment. The uplift acceptance criteria for beta57 includes fixes for severe recent regressions, perf and crash problems, data loss issues. To keep the risks minimal, let this fix ride the 58 train.
Attachment #8886230 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
(In reply to Ritu Kothari (:ritu) from comment #155)
> The uplift acceptance criteria for beta57
> includes fixes for severe recent regressions, perf and crash problems, data
> loss issues. To keep the risks minimal, let this fix ride the 58 train.

I'm not questioning your policy in general, but I do feel the addons situation is special in 57, and should perhaps carry extra considerations for the functionality loss due to disabling of legacy extensions, especially for something as low risk as this is, as assessed by Kris (the module owner of Web Extensions).
Flags: needinfo?(rkothari)
Tomislav, see the big picture. We, release management, are focusing on 57. As you know, this is a very important release.
As every patches that we take carry risks (even typo fixes), we stated that we are limiting the patches that we take and, in particular, we won't take new features on beta.

Here, even if I do agree with all your arguments, it is adding risks to the release.
Moreover, if we take this new API, why not others?
Part of the big picture is the marketing push we are doing for 57, which we hope will bring lots of new/returning users.

And for new users from Belgium, Estonia, France (and perhaps others), if they can't easily configure Firefox to work with (government issued/required) ID cards, they might just bounce back to Chrome, as noted in comment #150.


> Moreover, if we take this new API, why not others?

Because this is the only API that:
1) was already far enough in development/testing (slipped the 57 deadline), 
2) completely isolated from other code/browser/extensions functionality to make it not risky,
3) has higher impact on new users than existing ones (who already have an auth module configured).

If any other new API satisfied all these conditions, I would also consider asking for another exemption, but none of them do.
Flags: needinfo?(kaie)
For documentation:

This bug adds a new browser.pkcs11.* api, not supported by any other browser (other browsers have different ways of talking to smartcards). It provides ways to load PKCS#11 modules into the Firefox "Security Devices" configuration through an add-on.

A native manifest is required (see also bug 1386427) which points to the PKCS#11 module. It should follow the same format and rules as native messaging hosts manifests, except that the "type" key in the JSON file should be set to "pkcs11" rather than "stdio". Also important to note is that the "description" field in the JSON file is used to set the friendly name in the "security devices" dialog.

The API provides four new methods:

- isModuleInstalled(name): returns whether the module, registered under the manifest with the given name, is installed in the security devices. If the filename of the module matches but the description does not, then this will return true. If the description matches but the filename does not, it will return false. If a manifest of the given name was not found (or the addon does not have access), rejects the promise.
- installModule(name, flags): will install the module, registered under the manifest with the given name, into the security devices, using the data "description" field of the json manifest as the name and the "path" field as the filename of the PKCS#11 module.
- uninstallModule(name): remove the module again.
- getModuleSlots(name): retrieve an object containing a list of "slots" (this term is defined in the PKCS#11 standard) from the given PKCS#11 module, with the status of their tokens, in the following format:

[
  {
    "name": "slot name of first slot",
    "token": null // no token in this slot
  },
  {
    "name": "slot name of second slot",
    "token": {
      "name": "name of token",
      "manufacturer": "token manufacturer name",
      "HWVersion": "1.0", // PKCS#11-format version number (two 32-bit integer components)
      "SWVersion": "1.0", // same format
      "serial": "token serial number" // format defined by token spec
      "isLoggedIn": true or false // whether the token is logged on already
    }
  }
]
(In reply to Wouter Verhelst :wouter from comment #159)
> A native manifest is required (see also bug 1386427) which points to the
> PKCS#11 module. It should follow the same format and rules as native
> messaging hosts manifests, except that the "type" key in the JSON file
> should be set to "pkcs11" rather than "stdio". 

Also note that subdir name is "pkcs11-modules" on Linux, and "PKCS11Modules" on OSX.  On Windows, the manifest file needs to be pointed at by a registry key under "Software\Mozilla\PKCS11Modules\".

The api is desktop only, so not available on Android.
Hi there,

I have a question about deploying a WebExtension that uses the new 'pkcs11' API.

Since, it requires a registry key setting to work,
Can a WebExtension itself create such a key within restrictive permmission ?

If not how to achieve this ?

Should I tell my users to do this ?? (which seems unreasonable to my point of view ... )

Regards.
(In reply to BPER_ILEX from comment #161)
> Hi there,
> 
> I have a question about deploying a WebExtension that uses the new 'pkcs11'
> API.
> 
> Since, it requires a registry key setting to work,
> Can a WebExtension itself create such a key within restrictive permmission ?
> 
> If not how to achieve this ?
> 
> Should I tell my users to do this ?? (which seems unreasonable to my point
> of view ... )

I'm assuming you're installing the PKCS#11 module somehow? That installation process should install the manifest and registry key.
(In reply to Wouter Verhelst :wouter from comment #162)
> (In reply to BPER_ILEX from comment #161)
> > Hi there,
> > 
> > I have a question about deploying a WebExtension that uses the new 'pkcs11'
> > API.
> > 
> > Since, it requires a registry key setting to work,
> > Can a WebExtension itself create such a key within restrictive permmission ?
> > 
> > If not how to achieve this ?
> > 
> > Should I tell my users to do this ?? (which seems unreasonable to my point
> > of view ... )
> 
> I'm assuming you're installing the PKCS#11 module somehow? That installation
> process should install the manifest and registry key.

OK, Wouter; that is what I thought too.

A last question question:

What about on Mac & Linux. I suppose this a sub folder to create.
What is its expected name and location ?
(In reply to BPER_ILEX from comment #163)
> What about on Mac & Linux. I suppose this a sub folder to create.
> What is its expected name and location ?

The same directories as per Native Messenging Hosts[1], except you use "pkcs11-modules" (Linux) or "PKCS11Modules" (Mac) as the final directory name, rather than native-messaging-hosts or NativeMessagingHosts.

[1] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Native_messaging
I've added some docs:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/pkcs11
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Native_manifests#PKCS_11_manifests

As Tomislav previously suggested, I've now created a "native manifests" page, so there's one place where we talk about all these things.

Please let me know if we need anything else here.

I didn't know what the 'flags' argument to 'installModule' was. If you'd like to give me some details here I can add them.

I also wasn't able to test this API - I don't really like documenting stuff I've not tested, so if you know a relatively simple way I can install a PKCS #11 module on OS X for testing, especially a fake one that I can easily configure, I'd be happy to give that a go so I can test code samples and stuff like that.
Flags: needinfo?(w)
Hi Will,

(In reply to Will Bamberg [:wbamberg] from comment #165)
> I've added some docs:
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/pkcs11
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/
> Native_manifests#PKCS_11_manifests

That looks awesome, thanks.

> As Tomislav previously suggested, I've now created a "native manifests"
> page, so there's one place where we talk about all these things.
> 
> Please let me know if we need anything else here.

I noticed two minor details you might want to look at:
- If you link through to the permissions page from the pkcs11 api's page, the pkcs11 isn't listed there yet. This may be intentional?
- I notice that the section on native app manifests on the native manifests page is a straight copy from the native apps Page, as I would expect. However, some of the language there seems to still imply that the only type of existing manifests are the native app ones, despite the two sections below it. This could be confusing down the road. You may want to tweak that a little bit.

Also, while writing my own WebExtension version of my extension, I noticed that the security devices configuration allows for specifying a not fully qualified module filename. If you do that, then the system library pretty will be searched for the module name. On Windows, due to the explicit relative path handling, the security devices code will never end up getting a relative path though, so there that can't work.

I don't know whether this is accidental behaviour that we don't want to document, or whether we do want to document that and commit to it. Keeler, what's your opinion on that?

Beyond that, I think that's exactly what it needs to be.

> I didn't know what the 'flags' argument to 'installModule' was. If you'd
> like to give me some details here I can add them.

To be perfectly honest with you, I'm not entirely sure myself :)

It gets passed on, unmodified, to nsIPKCS11ModuleDB.addModule. I don't really know what all the possible module flags there are or what they mean. Probably the most common value may be 1<<28, which is PKCS11_PUB_READABLE_CERT_FLAG and which should be used if the module allows access to its certificates without logging in first. Beyond that, I don't know what the possible values are or if they've even been documented. Keeler, can you shed some light?

> I also wasn't able to test this API - I don't really like documenting stuff
> I've not tested, so if you know a relatively simple way I can install a PKCS
> #11 module on OS X for testing, especially a fake one that I can easily
> configure, I'd be happy to give that a go so I can test code samples and
> stuff like that.

There is a fake PKCS#11 module in the Firefox source which is used by unit tests. If you have a Mozilla build setup in your Mac, you can find it under security/manager/ssl/test/unit/pkcs11testmodule. Since it's only used for unit tests though, it's not shipped with Firefox so you need your own build.
Flags: needinfo?(w) → needinfo?(dkeeler)
(In reply to Wouter Verhelst :wouter from comment #166)
> > I didn't know what the 'flags' argument to 'installModule' was. If you'd
> > like to give me some details here I can add them.
> 
> To be perfectly honest with you, I'm not entirely sure myself :)
> 
> It gets passed on, unmodified, to nsIPKCS11ModuleDB.addModule. I don't
> really know what all the possible module flags there are or what they mean.
> Probably the most common value may be 1<<28, which is
> PKCS11_PUB_READABLE_CERT_FLAG and which should be used if the module allows
> access to its certificates without logging in first. Beyond that, I don't
> know what the possible values are or if they've even been documented.
> Keeler, can you shed some light?

I don't really know either. Odds are if someone was using these with the old api they already know whether or not they need to pass a value.

> > I also wasn't able to test this API - I don't really like documenting stuff
> > I've not tested, so if you know a relatively simple way I can install a PKCS
> > #11 module on OS X for testing, especially a fake one that I can easily
> > configure, I'd be happy to give that a go so I can test code samples and
> > stuff like that.
> 
> There is a fake PKCS#11 module in the Firefox source which is used by unit
> tests. If you have a Mozilla build setup in your Mac, you can find it under
> security/manager/ssl/test/unit/pkcs11testmodule. Since it's only used for
> unit tests though, it's not shipped with Firefox so you need your own build.

You might be able to grab this from our build infrastructure.
e.g. https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=390bc29173d8e3ff1f2ff13f7c87a9bfb73a1eb9&selectedJob=142802227 and then downloading the artifact target.common.tests.zip (you're looking for /xpcshell/tests/security/manager/ssl/tests/unit/pkcs11testmodule/libpkcs11testmodule.dylib I think)
Flags: needinfo?(dkeeler)
Thanks!

I did get the API working with the test PKCS module, thanks for the pointer there. So I can confirm that the code samples work. 

> - If you link through to the permissions page from the pkcs11 api's page,
> the pkcs11 isn't listed there yet. This may be intentional?

No, not intentional, and thank you for catching it. Should be fixed now.

> - I notice that the section on native app manifests on the native manifests
> page is a straight copy from the native apps Page, as I would expect.
> However, some of the language there seems to still imply that the only type
> of existing manifests are the native app ones, despite the two sections
> below it. This could be confusing down the road. You may want to tweak that
> a little bit.

I've made a few changes to this page,including glomming together the sections on manifest location, which were especially repetitive. I hope that's better now.
 
> > > I didn't know what the 'flags' argument to 'installModule' was. If you'd
> > > like to give me some details here I can add them.
> > 
> I don't really know either. Odds are if someone was using these with the old
> api they already know whether or not they need to pass a value.

I'm not wild about documenting this without explaining it, but I've left it in for now. As you say, if someone was used to passing in some flags with the old API, it may be useful to show that they can still do this with the new one.

I'm marking this dev-doc-complete but please do let me know if we need anything else.
Keywords: dev-doc-needed → dev-doc-complete
Should this maybe be listed on https://developer.mozilla.org/en-US/Firefox/Releases/58 under the "WebExtensions" section?
Flags: needinfo?(wbamberg)
Yes. I haven't filled in this page for 58 yet. I will do so soon, but won't fight you if you want to add it :).
Flags: needinfo?(wbamberg)
Hello all,

I wonder whether we can specify more than one module path
in the json file related to the module to load.

I tried this : "path" : ["C:\\Windows\\system32\\my_pkcs11_x64.dll", "C:\\Windows\\syswow64\\my_pkcs11_x86.dll"], ...

But sadly it doesn't work.

The problem is that I would like the installer to configure a SINGLE registry key and a SINGLE JSON file
even if Firefox is not installed. And in Windows 64 platforms, one can decide to install a 32 or 64 bits version of FF, we don't know in advance... 

How can I achieve this ?

Thanks.
(In reply to BPER_ILEX from comment #171)
> Hello all,
> 
> I wonder whether we can specify more than one module path
> in the json file related to the module to load.
> 
> I tried this : "path" : ["C:\\Windows\\system32\\my_pkcs11_x64.dll",
> "C:\\Windows\\syswow64\\my_pkcs11_x86.dll"], ...
> 
> But sadly it doesn't work.
> 
> The problem is that I would like the installer to configure a SINGLE
> registry key and a SINGLE JSON file

That's not possible. It will never be.

You install one JSON file per PKCS#11 module. This will always be the case.

> even if Firefox is not installed. And in Windows 64 platforms, one can
> decide to install a 32 or 64 bits version of FF, we don't know in advance... 
> 
> How can I achieve this ?

Once bug 1404624 has been fixed, you will be able to install two versions of the same JSON file, and Firefox will use the right one depending on the platform.

Until then, you have to install two JSON files with a different name, and have your extension use the correct name. There are two ways to do that:

- Use error handling to fall back to the second name:

    browser.pkcs11.installModule("pkcs11_32")
    .then(console.log("module installed (32 bit version)"))
    .catch(browser.pkcs11.installModule("pkcs11_64"))
    .then(console.log("module installed (64 bit version)"))
    .catch(console.log("module could not be installed"));

- Use browser.runtime.getPlatformInfo() to figure out which platform you're on, and use that to load the right extension:

    browser.runtime.getPlatformInfo()
    .then(function(p) {
      if(p.os === "win")
        if(p.arch === "x86-32") {
          browser.pkcs11.installModule("pkcs11_32");
        } else {
          browser.pkcs11.installModule("pkcs11_64");
        }
      }
    }).catch(console.log("module could not be installed"));

The second is a bit clearer IMO, but it's up to you what you do really.
Hello Wouter,

thanks for providing snippet code !

We'll try this.

Regards.
if(p.os === "win")
        if(p.arch === "x86-32") {
          browser.pkcs11.installModule("pkcs11_32");
        } else {
          browser.pkcs11.installModule("pkcs11_64");
        }

in windows 64 bit, any combination of bitness 32/64 of the browser and the pkcs11-module is allowed.

also starting from firefox 57, earlier versions of the Eid middleware are not recognized and the addon needs to be disabled to be able to login successfully. this forces the user to reinstall the eID software (i didn't check if there's already a version available for the firefox 57+ addon.)
(In reply to Pieter Janssens from comment #174)
> if(p.os === "win")
>         if(p.arch === "x86-32") {
>           browser.pkcs11.installModule("pkcs11_32");
>         } else {
>           browser.pkcs11.installModule("pkcs11_64");
>         }
> 
> in windows 64 bit, any combination of bitness 32/64 of the browser and the
> pkcs11-module is allowed.

Eh, nope. You can't load a 32-bit DLL into a 64-bit process space (how would you do that? The hardware doesn't support it). Before bug 1396030 is fixed, you absolutely need to make sure that you load the right module.

> also starting from firefox 57, earlier versions of the Eid middleware are
> not recognized and the addon needs to be disabled to be able to login
> successfully.

This is not correct. From 58 you do need the more recent version if you want the addon to work, but nothing beyond that.

Older installations of the browser that were already configured will continue to work.

> this forces the user to reinstall the eID software (i didn't
> check if there's already a version available for the firefox 57+ addon.)

If you're talking about the BeID middleware, then yes (but this bug is not about that).
Hi,

We have a very serious problem with thousand of users and this number increase day by day with update to Firefox 58.

Explanation :
We have a previous Extension version which configure a PKCS11 peripherical until Firefox 56. In fact, the update to Firefox 57, let the peripherical working with Extension disable. We had a new extension version ready for online update in time for Firefox 58 release.
During the update to Firefox 58, the old extension version is remove as expected but it seems remove our PKCS11 peripherical too. After that, the online update to the new extension version is realized. But peripherical could not be configured because the corresponding system installer need to be executed in parallel due to new Web Extension architecture.

Did you observe the same behaviour ? What we could do to avoid it ?

Thanks in advance,
This is not a support forum for general questions. Please use Discourse or the mailing lists as listed here: https://wiki.mozilla.org/Add-ons#Get_in_touch
Sorry, but I don't think it is a support question.

We are directly impacted by this thread since FF55. We follow it for a long time and follow all the work to be able to have our users stable in Firefox usage.

I suggest to verify if there is a bug or a unknown behaviour of this new implementation against extension version supression before its online update. Perhaps, the call of the old extension architecture destroy configuration during it self removing process.

Since FF56, 3 differents implementation of Extension succeeds. Perhaps, it is our understanding which is wrong.

Our internal analysis is in progress.
Hello,

I can confirm this behavior (on windows),
when firefox updates from version 57 to 58 it removes all pkcs11 modules that were attached to firefox.
It doesn't matter if those modules were attached using our extention, or manually, they always get removed.

(Just to be clear: also happens when no eid extention was installed)


Can anyone confirm if this a bug, or if this was done by design?
I filed a new bug for this issue here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1433464
Depends on: 1434204
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: