1260626 - Add mechanism to allow tests to opt out of pkcs11testmodule's cyclic token insertions and removals

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

The "pkcs11testmodule" is a testing PKCS #11 module that simulates a token being inserted and removed from a slot every 50ms. This behaviour is useful for what it was originally introduced for (see Bug 1038913 and test_pkcs11_insert_remove.js) and for other tests such as test_pkcs11_no_events_after_removal.js, but is annoying for other PKCS #11 tests.

Some examples:
 - https://hg.mozilla.org/mozilla-central/file/cdcc256acea1/security/manager/ssl/tests/unit/test_pkcs11_slot.js#l38
 - https://hg.mozilla.org/mozilla-central/file/cdcc256acea1/security/manager/ssl/tests/unit/test_pkcs11_insert_remove.js#l35

If possible, changes should be made to allow tests that don't want this cyclic behaviour to avoid it somehow, such as:
(Dana Keeler [:keeler] (use needinfo?) from Bug 1259149 comment #2)
> In another patch (that probably won't get merged since I went with another
> approach) I explored making this setup a bit easier to work with. I think we
> could essentially control whether or not the test module pretends the token
> has been inserted/removed via an environment variable. That way, the tests
> that aren't interested in this aspect don't have to deal with it.

Comment 2

[:Cykesiopka]

Attachment: Bug 1260626 - Add extra slot and token to pkcs11testmodule that is always present.

This allow tests to opt out of pkcs11testmodule's current cyclic token
insertions and removals, which is useful for some tests, but an annoyance for
others.

Review commit: https://reviewboard.mozilla.org/r/89428/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/89428/

Comment 3

[:Cykesiopka]

Attachment: Bug 1260626 - Add helper function in head_psm.js to load and unload the test PKCS11 module.

This helps cleanup some tests.

Review commit: https://reviewboard.mozilla.org/r/89430/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/89430/

Comment 4

[:Cykesiopka]

Attachment: Bug 1260626 - Take advantage of the always present test token.

Now that we have an always present test token, we can add more tests, and make
other tests not intermittently fail.

Review commit: https://reviewboard.mozilla.org/r/89432/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/89432/

Comment 5

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8805770 [details]
Bug 1260626 - Add extra slot and token to pkcs11testmodule that is always present.

https://reviewboard.mozilla.org/r/89428/#review88960

LGTM, but see the comment below (in particular, that might require a follow-up bug).

::: security/manager/ssl/tests/unit/pkcs11testmodule/pkcs11testmodule.cpp:81
(Diff revision 1)
>    if (!pulCount) {
>      return CKR_ARGUMENTS_BAD;
>    }
>  
>    if (pSlotList) {
> +    if (*pulCount < 2) {

Shouldn't this check depend on the same logic that determined the value of pulCount from the previous call? (Although, now that I think about it, there's nothing that prevents tokenPresent from changing between two successive calls to this function, which might be an issue we need to address.)

Comment 6

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8805771 [details]
Bug 1260626 - Add helper function in head_psm.js to load and unload the test PKCS11 module.

https://reviewboard.mozilla.org/r/89430/#review89032

Cool - r=me. Just the one suggestion.

::: security/manager/ssl/tests/unit/head_psm.js:813
(Diff revision 1)
> +  let pkcs11 = Cc["@mozilla.org/security/pkcs11;1"].getService(Ci.nsIPKCS11);
> +  do_register_cleanup(() => {
> +    try {
> +      pkcs11.deleteModule("PKCS11 Test Module");
> +    } catch (e) {
> +      // The test module may have already been deleted prior to this cleanup

Looks like half of the tests that will be calling this are expecting deleteModule to succeed (and the other half are expecting it to maybe fail or to be unnecessary). Perhaps we should add a parameter to loadPKCS1TestModule to enable both scenarios?

Comment 7

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8805772 [details]
Bug 1260626 - Take advantage of the always present test token.

https://reviewboard.mozilla.org/r/89432/#review89036

Great - r=me.

Comment 8

[:Cykesiopka]

Comment on attachment 8805770 [details]
Bug 1260626 - Add extra slot and token to pkcs11testmodule that is always present.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/89428/diff/1-2/

Comment 9

[:Cykesiopka]

Comment on attachment 8805771 [details]
Bug 1260626 - Add helper function in head_psm.js to load and unload the test PKCS11 module.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/89430/diff/1-2/

Comment 10

[:Cykesiopka]

Comment on attachment 8805772 [details]
Bug 1260626 - Take advantage of the always present test token.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/89432/diff/1-2/

Comment 11

[:Cykesiopka]

Comment on attachment 8805770 [details]
Bug 1260626 - Add extra slot and token to pkcs11testmodule that is always present.

https://reviewboard.mozilla.org/r/89428/#review88960

> Shouldn't this check depend on the same logic that determined the value of pulCount from the previous call? (Although, now that I think about it, there's nothing that prevents tokenPresent from changing between two successive calls to this function, which might be an issue we need to address.)

I think I addressed this, but it would be good if you could take another look.
I'll file a follow-up bug later.

Comment 12

[:Cykesiopka]

keeler: See comment 11.

Comment 13

[Dana Keeler (she/her) [:keeler]]

Yes, that looks correct to me. In any case, I don't think it matters for the moment, because NSS always calls that function with limitToTokensPresent set to false (in the lib node, not necessarily the cmd code, but that shouldn't affect this). If that ever changes, though, there might be an issue.

Comment 14

[:Cykesiopka]

Thanks!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f39fdc04b559451a987fec1a11a8357197ea79e
The failures look unrelated.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/851d4bd145a4
Add extra slot and token to pkcs11testmodule that is always present. r=keeler
https://hg.mozilla.org/integration/autoland/rev/34bca938935a
Add helper function in head_psm.js to load and unload the test PKCS11 module. r=keeler
https://hg.mozilla.org/integration/autoland/rev/e3ced3657d49
Take advantage of the always present test token. r=keeler

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/851d4bd145a4
https://hg.mozilla.org/mozilla-central/rev/34bca938935a
https://hg.mozilla.org/mozilla-central/rev/e3ced3657d49

Comment 17

[Gary Chen [:xeonchen]]

Hi Cykesiopka,

I cannot compile m-c after this patch was landed, the error is:

> c:/mozilla-source/gecko/security/manager/ssl/tests/unit/pkcs11testmodule/pkcs11testmodule.cpp(104): error C2001: newline in constant
> c:/mozilla-source/gecko/security/manager/ssl/tests/unit/pkcs11testmodule/pkcs11testmodule.cpp(106): error C2146: syntax error: missing ';' before identifier 'CK_RV'```

Looks like encoding problem?
The platform I'm using is Windows 7 x64 (Traditional Chinese).

Comment 18

[:Cykesiopka]

(In reply to Gary Chen [:xeonchen] from comment #17)
> Hi Cykesiopka,
> 
> I cannot compile m-c after this patch was landed, the error is:
> 
> > c:/mozilla-source/gecko/security/manager/ssl/tests/unit/pkcs11testmodule/pkcs11testmodule.cpp(104): error C2001: newline in constant
> > c:/mozilla-source/gecko/security/manager/ssl/tests/unit/pkcs11testmodule/pkcs11testmodule.cpp(106): error C2146: syntax error: missing ';' before identifier 'CK_RV'```
> 
> Looks like encoding problem?
> The platform I'm using is Windows 7 x64 (Traditional Chinese).

Looks like this is filed as Bug 1315152 now.

Comment 19

[:Cykesiopka]

(In reply to :Cykesiopka from comment #11)
> I'll file a follow-up bug later.

Bug 1316293.