1011625 - Add tests for S/MIME

Status: RESOLVED FIXED

(MailNews Core :: Security: S/MIME, defect)

Description

[Joshua Cranmer [:jcranmer]]

Basic things to test:
1. Create signed messages
2. Create encrypted messages to one recipient + self [for FCC/sent]
3. Create encrypted messages to multiple recipients
4. Sign+encrypt, not encrypt+sign
5. Correctly indicate signed/encrypted status of:
5a. Valid signature
5b. Valid signature, different from From
5c. Valid signature, expired (but valid when message was sent)
5d. Valid encryption
5e. Sign-and-encrypted
5f. Encrypted-and-signed
5g. S/E/S triple-wrap
5h. E/S/E triple-wrap
5i. Non-top-level signature/encryption information
5j. Sign-and-encrypt with other stuff in the middle

Note that some of the subparts of 5 correspond to things for which we don't have UI necessarily to distinguish.

Comment 1

[Joshua Cranmer [:jcranmer]]

Attachment: Preliminary work

This is nowhere near checkin ready, but this gives a functional start to some of the ideas I have for testing, as well as a working test that we can produce a correct multipart/signed S/MIME message.

Comment 2

[Joshua Cranmer [:jcranmer]]

For notes: this patch relies on those in bug 1020696 to apply and work correctly.

Comment 3

[Joshua Cranmer [:jcranmer]]

Attachment: More preliminary work

Comment 4

[Joshua Cranmer [:jcranmer]]

Attachment: Working patch, not commitable

So, this patch is still preliminary, and it's relatively low importance in the grand scheme of things, but I'd like some feedback on the basic mechanics.

There are basically two approaches to generating data: we can construct data at runtime in the tests, or we can preconstruct data (during the build, as this patch does, or as a precommit step, as will likely need to be done). By this patch I've ripped out the attempts to construct data at runtime--it requires calling NSS manually, and the NSS APIs themselves are not for the faint of heart. (You can see what it looks like in the obsolete patches)

Instead, it builds on the certificate generation logic that m-c uses for its tests. When I started this, the certificates were generated during the build step. Nowadays, the logic to do that generation is I believe still in the tree, but the certificates are checked in because generating certificates takes quite a lot of time. There's a small patch to m-c needed to make this code work (I need to enable email addresses in subjectAltName), which is not yet included.

As for generating the S/MIME messages themselves, out of the various approaches, I've gone for a simplest approach: I use the openssl smime tool via the command line to generate these messages based on an email template (see pysmime.py for the details). This renders it difficult to use this as part of the regular build system. NSS does not generate the S/MIME command-line tool in the configuration we build, and driving those APIs manually is, again, annoying. An alternative approach is generating the CMS blobs manually ourselves: the python code in m-c for certificate generation does this. This is difficult for us, however, because we don't have access to any symmetric key ciphers. Also, generating ASN.1 DER is really annoying (see pypkcs12.py for some disabled code that tries to do it--it didn't work, so I again resorted to openssl).

In addition to generating messages and certificates, we also need to import the certificates and private keys into the profile for testing. We can't use the ASCII-encoded format for the private key, so I end up packing it into a PKCS12 file (something we can import) as yet another step.

In short, the feedback I'm looking for:
1. Is this an acceptable way of generating keys and certificates, and importing them for use, for testing purposes?
2. Is the process of generating S/MIME messages for test acceptable?
3. Does the complexity of the test framework outweigh the benefits of testing?
4. If the answers to 1 or 2 is no, then what alternative methods should be pursued?

Comment 5

[Jorg K (CEST = GMT+2)]

Comment on attachment 8914156 [details] [diff] [review]
Working patch, not commitable

I'm not 100% sure which level of profoundness the feedback should have. Also, be aware that you're asking a pragmatist, not a purist.

My answers you be:
1. Is this an acceptable way of generating keys and certificates, and importing them for use, for testing purposes?
2. Is the process of generating S/MIME messages for test acceptable?
3. Does the complexity of the test framework outweigh the benefits of testing?

Yes, yes and no. Or maybe the answer to 1. and 2. should be to pre-can them and check them in, see below.

Re. 3: Yes, it is lamentable that the text framework is so complex (as is encrypted e-mail in general), but there is no alternative since not having test is not a comfortable position.

Re. 1 and 2 I have the following questions:
M-C: "Nowadays, ..., but the certificates are checked in"
Should we do the same? Am I misunderstanding or would that further simplify the test?

"openssl ... renders it difficult to use this as part of the regular build system".
So would storing/checking in the certificates avoid that? Is openssl present on all systems? I have it as part of XAMPP, which is an Apache implementation for Windows to run local servers.

"see pypkcs12.py for some disabled code that tries to do it"
I can't locate that code in the most recent patch, there is only:
#pyasn1.debug.setLogger(pyasn1.debug.Debug('all'))

Would it be an option to pre-can and check in everything, certificates and S/MIME messages? A certificate could be made valid for 500 years and I assume TB (or mankind) won't exist then any more. A big comment could also advise future generations to regenerate everything at that point. Alternatively, make the certificates valid for one year and provide a facility to regenerate them periodically.

Comment 6

[Jorg K (CEST = GMT+2)]

Or maybe if we resort to storing everything, that should have been an f-, but anyway, you have my thoughts.

Comment 7

[Kent James (:rkent)]

jcranmer said:
===
In short, the feedback I'm looking for:
1. Is this an acceptable way of generating keys and certificates, and importing
them for use, for testing purposes?
2. Is the process of generating S/MIME messages for test acceptable?
3. Does the complexity of the test framework outweigh the benefits of testing?
===

I would also agree yes, yes, and no.

But let me make sure that I understand this. You are using openssl to generate test certificates, and then using those test certificates in the actual tests. This is fine as long as the process to generate those certificates is clearly documented and runnable by someone with the skill level of myself, Jorg, or Magnus. The existing documentation is not quite there.

As to when to generate those certificates, an automated process is certainly preferable to a manual process whose details could degrade over time. But if it is time consuming, I would not want that a part of standard builds or tests. Perhaps what we need is a "supertest" mode that could run slow processes, and would be run less often. There are also other slow tests (like tests of very large mbox files) that could also be put in the periodic "slow" process.

But a caveat. S/MIME's future is presently in doubt as I understand it. There is no established process for certificate providers to use in generating the certificates. m.d.s.policy sometimes suggests that the fields needed for S/MIME be dropped completely because of little interest in the mechanics of the certificate process.

S/MIME is certainly a good thing that a more robust Thunderbird might promote, including the certificate process, but we are not there yet. I really have no clue myself on whether S/MIME is actively used by folks or not. Better testing of S/MIME would certainly be a part of a more active support for it, but is not really enough by itself.

Comment 8

[Magnus Melin [:mkmelin]]

@kaie: some work done here. Have a look, could be at least a start for getting automated test coverage for S/MIME

Comment 9

[Kai Engert [:KaiE:]]

Joshua, thanks for doing this groundwork. Do you remember which parts you considered to be still missing?

I think we shouldn't use openssl, if possible. As Jorg noted, it might not be available on all systems, especially not on Windows. And I think we don't need it.

NSS has tools for certificate generation and for CMS message generation, which are used by the NSS test suite. A Firefox build already produces the certutil and pk12util toolss, available in obj/dist/bin. S/MIME is base64-encoded CMS messages combined with headers and MIME. NSS has a cmsutil tool. To get from CMS to S/MIME, you just base64 encode and add the MIME wrappers. Using NSS tools means no external dependencies.

Let's start with dynamic certificate generation as part of the build, by default. If that needs to be optimized for execution time, by checking in pre-generated certs, let's handle that later. (Potentially NSS could distribute a few test certificates, e.g. valid for 3 years. Given that applications are supposed to frequently pick up newer versions of NSS, that might be sufficient for refreshing.)

We don't need many certificates, and the time necessary for that should be acceptable. Note that NSS already has a large test suite, which tests all the crypto details, and it also tests CMS message generation and verification. We can rely on that and focus on S/MIME message integration, and for that, avoiding the slower and larger keysizes should be OK.

We need a CA, an intermediate CA, and certs for maybe 4 email addresses, alice, bob, david, eve, that's what the NSS CMS tests do.

We should create the kind of messages that were mentioned in the initial comment. Also, we should have a few nested messages. For example, an unsigned message from Alice to Eve, which attaches/forwards a signed message from Bob. The message seen by Eve shouldn't show a signature. This test ensures that nested CMS status isn't propagated to the wrong display level.

I see the patch creates messages, stores them into a folder, and loads messages, then queries user interface elements to check the S/MIME status of the UI. Is that correct? Does that mean we don't require mozmill for testing the UI?

Comment 10

[Joshua Cranmer [:jcranmer]]

My original work on this patch followed what mozilla-central's certificate tests were doing at the time (I haven't kept up to see if they're still doing this). That involved creating the certificates and some of the encrypted blobs at build time using some python-heavy code to do the generation. I actually reused as much of the infrastructure as I could, going so far as to drop in some of the other py-asn1 modules for the relevant CMS blobs. (I really, really hate ASN.1, have I mentioned that?)

Certificate generation isn't the hard part; I literally reused the GenerateCertificate blobs from mozilla-central. Generating the pkcs12 key file was more annoying; I resorted to OpenSSL there mostly because I couldn't get a working output file using the ASN.1 generation stuff. The hard part is actually generating the S/MIME messages themselves: the way the python system for generating keys/certificates works is to actually make manual calls to the encryption routines to generate the data. (The RNG is seeded, IIRC, to make bit-identical results). But there's nothing in the mozpython distribution to do the necessary symmetric encryption legwork.

The way I envisioned the framework working is that there is a build script that takes a fully-decrypted email variant and replaces the bodies of custom MIME parts with CMS blobs. That's pysmime.py, absent some missing functionality and the fact that it currently uses the OpenSSL for generation.

As for missing functionality:

1. There's no way to select between multipart/signed and application/pkcs7-mime in message generation. Testing both is definitely essential for correctness.
2. There's no mechanism to indicate a certs-only attachment. Definitely lower priority, though.
3. There's also no mechanism for forcing a particular encryption method, hashing method, or signing time. This is also lower priority, and only really necessary if we're going to be doing more custom policy decisions than we do at present.
4. This patch doesn't implement any tests of S/MIME composition, only decoding. I don't think it's missing much pieces for that, though.

Looking back at my original comments for tests, I see some other things that ought to be tested in the long run:

• EAI/IDN handling. I actually have no idea what the expected practice for certificates in this space is.
• Mozmill plumbing tests to make sure we're actually displaying security status, or propagating the status in compose.
• Mozmill tests for configuring use of certificates. I think we've had some bad regressions in this space.

We don't need to use mozmill for tests that are merely checking "did we encrypt the message correctly" or "do we compute the right security status for this message"; the xpcshell tests can easily reuse the same API we use to indicate it to the message view. And, personally, I much prefer xpcshell over mozmill for testing.

Comment 11

[Kai Engert [:KaiE:]]

Regression bug 1528615 demonstrates that this work should get a high priority.

Because I had an immediate need for testcases, I've extended the NSS script to create certificates and messages.
I'd like to attach some work-in-progress, which can be used for interactive testing, while we don't have it automated yet.

Comment 12

[Kai Engert [:KaiE:]]

Attachment: smime-mailbox.tar.gz

Download, extract into the "Mail/Local Folders" subdirectory of a Thundebird profile.

Comment 13

[Kai Engert [:KaiE:]]

Attachment: db.tar.gz

Database with certificates for the messages in the previous mailbox attachment. Extract into a test profile folder, overwriting the existing files.

Comment 14

[Kai Engert [:KaiE:]]

Attachment: nss-gen.patch

Patch against NSS used to generate the certificates and test messages.
(As part as running the NSS S/MIME tests, which aren't really S/MIME tests, but rather CMS tests (S/MIME without MIME).)

Comment 15

[Kai Engert [:KaiE:]]

In order to perform manual testing, do the following steps:

• download both attachment 9044666 [details] and attachment 9044667 [details]
• create a test thunderbird profile, quit thunderbird
• extract the db.tar.gz archive into the main thunderbird profile
  (the one that contains files like prefs.js and cert9.db)
• extract the smime-mailbox.tar.gz archive into the "Mail/Local Folders" subdirectory
• start thunderbird
• go to local folders, click the folder named "smime-mailbox"
• click on each of the messages (slowly):
  • messages that start with a subject BAD should get a
    broken signature icon (letter with large red X)
  • for all other messages,
    ensure there is NO letter with a large red X.
    If the subject mentions "signed", you should get a letter with a small red seal inside it.
    If the subject mentions "envoloped", you should see a padlock

Comment 16

[Kai Engert [:KaiE:]]

Attachment: 1011625-merged-20190221.patch

This is Joshua's patch merged to current trunk, plus adjustments that were necessary. A lot has changed during the past years...

work-in-progress

Comment 17

[Kai Engert [:KaiE:]]

FYI, Firefox tests use pre-generated certificates that must be refreshed and committed yearly. They just recently tripped over that (no reminder...) in bug 1525191.

It seems reasonable to use the pre-generate approach for TB, too (but have a reminder, to do it before they expire).

I'd like to avoid the openssl dependency. The NSS test suite already creates certificates of many flavors, and it already creates test CMS messages, to ensure this functionality isn't regressing.

It's a small amount of work on top of that, to also generate what we require for TB. We could change NSS to place a folder with the data that TB requires in a separate folder. Then, once a year, someone would run the NSS test suite, copy over that folder into the TB tree, commit, done.

Comment 18

[Magnus Melin [:mkmelin]]

Yes, updating once a year is quite reasonable.
Not sure what reminder you had in mind, but I think the tests should check current date, and if it's over a hard coded end date, error out giving the prompt to update the certs and how.

Comment 19

[Kai Engert [:KaiE:]]

Attachment: 1011625-v6.patch

Comment 20

[Kai Engert [:KaiE:]]

Attachment: mailnews-test-data-smime.tar.gz

This archive contains the test data that was produced as described in the README file, by the enhanced script from bug 1529950.

I think it's better to not attach it as a patch. To use it:

cd mailnews/test/data
tar xzf /tmp/mailnews-test-data-smime.tar.gz

Comment 21

[Kai Engert [:KaiE:]]

Attachment: 1011625-v7.patch

v6 contained a block that's already on trunk. v7 fixes that.

Comment 22

[Joshua Cranmer [:jcranmer]]

(In reply to Kai Engert (:kaie:) from comment #17)

I'd like to avoid the openssl dependency. The NSS test suite already creates certificates of many flavors, and it already creates test CMS messages, to ensure this functionality isn't regressing.

The only reason I had openssl was because we weren't building the NSS tools when building TB. You've apparently fixed that now, so there should be no reason to use openssl.

Comment 23

[Kai Engert [:KaiE:]]

try build looks good:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=1c81810a2b5fcf8760a6cc09058444225873e8bc

Comment 24

[Kai Engert [:KaiE:]]

Attachment: 1011625-jcranmer-v7.patch - reused code from Joshua

Joshua, I'd like to credit you for the majority of the patch, which I reused, and which only required minor tweaks.

This is the subset of the v7 patch that's mostly from you.

Because it works, r+ from me.

Comment 25

[Kai Engert [:KaiE:]]

Attachment: 1011625-kaie-v7.patch

This part still contains snippets from Joshua (mostly smimeHeaderSink), but I rewrote most parts.

Joshua, would you like to review it?

I faced the challenge that the functions need to be async, because I needed to use "await". And I must wait until the message has really been copied to the inbox, prior to executing the test.

Also, if everything runs in parallel, the copying of multiple messages to the inbox might result in a non-predictable order of messages.

Also, I must ensure that smimeHeaderSink isn't being used in parallel, so another need to serialize.

My best idea was to use a chain of promises.

(There was also some c++ code, too, I've moved that to bug 1530099.)

Comment 26

[Joshua Cranmer [:jcranmer]]

Comment on attachment 9046125 [details] [diff] [review]
1011625-kaie-v7.patch

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

I do think the test suite could be more comprehensive, and the generation of the test emails a little more automatic, but I don't think those are reasons to hold this work up.

::: mailnews/mime/test/unit/test_smime_decrypt.js
@@ +59,5 @@
> +      await waiter.promise;
> +      waiter.promise = null;
> +      //dump("===> done waiting: " + msg.filename + "\n");
> +  }
> +  

There's a lot of trailing whitespace in this file that should get cleaned up.

@@ +123,5 @@
> +
> +var messages = [
> +  { filename: 'alice.env.eml',
> +    enc: true, sig: false, sig_good: false },
> +  { filename: 'alice.dsig.SHA1.multipart.bad.eml',

Some guide to what these email names mean would be helpful.

@@ +188,5 @@
> +    } else {
> +        nextResolver = PromiseUtils.defer();
> +    }
> +
> +    check_smime(messages[i], waiter, nextResolver);

You can clean this entire bit up with

async function check_smime_message() {
  for (let message of messages)
    await check_smime(messages[i]);
}

can you not?

::: mailnews/test/data/smime/README.md
@@ +23,5 @@
> +HOST=localhost DOMSUF=localdomain ./all.sh
> +cd ../../tests_results/security/localhost.1/sharedb/smime/
> +
> +Copy all files from the ./tb subdirectory to the TB tree, e.g.
> +cp -v ./tb/* $HOME/your-mozilla-tree/comm/mailnews/test/data/smime/

Thank you for including the instructions on how to regenerate this data.

Comment 27

[Magnus Melin [:mkmelin]]

Comment on attachment 9046123 [details] [diff] [review]
1011625-jcranmer-v7.patch - reused code from Joshua

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

::: mailnews/mime/test/unit/head_mime.js
@@ +69,5 @@
> +        Object.getOwnPropertyDescriptor(stubHeaderSink, name));
> +    }
> +  }
> +
> +  var msgWindow = Cc["@mozilla.org/messenger/msgwindow;1"]

nit: make this too a |let|

::: mailnews/test/resources/smimeUtils.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/*

Double star for documentation comments

/**
 *
 */

@@ +6,5 @@
> + * This file provides some utilities for helping run S/MIME tests.
> + */
> +
> +var EXPORTED_SYMBOLS = [
> +  'SmimeUtils'

please prefer double quotes

Also, since this file is a module, name it .jsm

@@ +11,5 @@
> +];
> +
> +//Components.utils.import("resource://gre/modules/Task.jsm");
> +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> +Components.utils.import("resource://testing-common/mailnews/MockFactory.js");

Does this still work? SHouldn't it be ChromeUtils.import?

@@ +14,5 @@
> +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> +Components.utils.import("resource://testing-common/mailnews/MockFactory.js");
> +
> +var Cc = Components.classes;
> +var Ci = Components.interfaces;

Cc and Ci should be globally available nowadays. Use them above. Y

@@ +53,5 @@
> +  }
> +};
> +
> +var SmimeUtils = {
> +  ensureNSS: function () {

nit, no space after "function": function()

Comment 28

[Magnus Melin [:mkmelin]]

Comment on attachment 9046125 [details] [diff] [review]
1011625-kaie-v7.patch

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

::: mailnews/mime/test/unit/test_smime_decrypt.js
@@ +14,5 @@
> +var gInbox;
> +
> +var smimeDataDirectory = "../../../data/smime/";
> +
> +var gMessageGenerator = new MessageGenerator();

not used

@@ +59,5 @@
> +      await waiter.promise;
> +      waiter.promise = null;
> +      //dump("===> done waiting: " + msg.filename + "\n");
> +  }
> +  

2 space indention please

@@ +72,5 @@
> +  await promiseCopyListener.promise;
> +  promiseCopyListener = null;
> +
> +  let numExpected = 1;
> +  if (msg.enc && msg.sig) ++numExpected;

nit: 
if (msg.enc && msg.sig) {
  numExpected++
}

@@ +93,5 @@
> +  Assert.ok(contents.includes("<html>"));
> +  
> +  await sinkPromise;
> +
> +  let r = smimeHeaderSink._results 

nit: missing trailing ;

@@ +121,5 @@
> +  }
> +}
> +
> +var messages = [
> +  { filename: 'alice.env.eml',

prefer double quotes

@@ +181,5 @@
> +  let nextResolver = null;
> +
> +  let iMax = messages.length;
> +
> +  for (let i = 0; i < iMax; ++i) {

i++ is more kosher

@@ +183,5 @@
> +  let iMax = messages.length;
> +
> +  for (let i = 0; i < iMax; ++i) {
> +    if (i == (iMax -1)) {
> +        nextResolver = gCleanupWaiter;

here too some mixed indention. 2 space please

::: mailnews/test/data/smime/README.md
@@ +19,5 @@
> +make nss_build_all
> +export NSS_CYCLES=sharedb
> +export NSS_TESTS=smime
> +cd tests
> +HOST=localhost DOMSUF=localdomain ./all.sh

what does this do? I don't see HOST being used later

@@ +23,5 @@
> +HOST=localhost DOMSUF=localdomain ./all.sh
> +cd ../../tests_results/security/localhost.1/sharedb/smime/
> +
> +Copy all files from the ./tb subdirectory to the TB tree, e.g.
> +cp -v ./tb/* $HOME/your-mozilla-tree/comm/mailnews/test/data/smime/

Perhaps you could insclude a schell script for this and refer to that in the README.
Regarding the inscructions, can we instead of cding to more parametrization. Does this work:

NSSBUILD_DIR=nssbuild
MOZILLA_TREE_ROOT=$HOME/your-mozilla-tree

mkdir $NSSBUILD_DIR
cd nssbuild
hg --cwd=$NSSBUILD_DIR clone https://hg.mozilla.org/projects/nspr/
hg --cwd=$NSSBUILD_DIR clone https://hg.mozilla.org/projects/nss/
# optional: cd nspr; hg update -r latest-nspr-release-tag; cd ..
# optional: cd nss; hg update -r latest-nss-release-tag; cd ..
export USE_64=1
cd nss
make -C $NSSBUILD_DIR/nss nss_build_all
export NSS_CYCLES=sharedb
export NSS_TESTS=smime
cd tests
HOST=localhost DOMSUF=localdomain ./all.sh
 
Copy all files from the ./tb subdirectory to the TB tree, e.g.
cp -v $NSSBUILD_DIRtests_results/security/localhost.1/sharedb/smime/tb/* $MOZILLA_TREE_ROOT/comm/mailnews/test/data/smime/

Comment 29

[Kai Engert [:KaiE:]]

(In reply to Joshua Cranmer [:jcranmer] from comment #26)

I do think the test suite could be more comprehensive,

True. Let's start with this initial set, which also focuses on the code required for testing. We can add more test cases in follow-up bugs.

and the generation of
the test emails a little more automatic, but I don't think those are reasons
to hold this work up.

Once we have cmsutil being built as part of TB (close to being done), we can use it together with the checked in certificates to create more emails in follow-up bugs.

::: mailnews/mime/test/unit/test_smime_decrypt.js
There's a lot of trailing whitespace in this file that should get cleaned up.

done

• { filename: 'alice.env.eml',
• { filename: 'alice.dsig.SHA1.multipart.bad.eml',

Some guide to what these email names mean would be helpful.

done, added comments

Comment 30

[Kai Engert [:KaiE:]]

(In reply to Magnus Melin [:mkmelin] from comment #27)

• var msgWindow = Cc["@mozilla.org/messenger/msgwindow;1"]

nit: make this too a |let|

done

Double star for documentation comments

done

+var EXPORTED_SYMBOLS = [

• 'SmimeUtils'

please prefer double quotes

done

Also, since this file is a module, name it .jsm

done

+Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
+Components.utils.import("resource://testing-common/mailnews/MockFactory.js");

Does this still work? SHouldn't it be ChromeUtils.import?

Changed to var {} = ChromeUtils.import

+var Cc = Components.classes;
+var Ci = Components.interfaces;

Cc and Ci should be globally available nowadays. Use them above. Y

removed

+var SmimeUtils = {

• ensureNSS: function () {

nit, no space after "function": function()

changed in all places

Comment 31

[Kai Engert [:KaiE:]]

(In reply to Magnus Melin [:mkmelin] from comment #28)

+var gMessageGenerator = new MessageGenerator();

not used

removed

2 space indention please

done

nit:
if (msg.enc && msg.sig) {
numExpected++
}

ok

• let r = smimeHeaderSink._results

nit: missing trailing ;

done.

+var messages = [

• { filename: 'alice.env.eml',

prefer double quotes

done

• for (let i = 0; i < iMax; ++i) {

i++ is more kosher

I'm changing it, but I'm used to ++i, because it's more efficient in C++:
https://stackoverflow.com/questions/24901/is-there-a-performance-difference-between-i-and-i-in-c

i++ means: "Take the value of i as an expression, then increment it, and the result is the new expression."

++i means: "Increment it, and use the new value as an expression."

There's also some talk about it in JS:
https://stackoverflow.com/questions/1546981/post-increment-vs-pre-increment-javascript-optimization

• if (i == (iMax -1)) {

•    nextResolver = gCleanupWaiter;
  

here too some mixed indention. 2 space please

done

+HOST=localhost DOMSUF=localdomain ./all.sh

what does this do? I don't see HOST being used later

An internal requirement of the existing NSS test suite that we're reusing here.
It passes the variables to the all.sh script only.
They are required inside the NSS test suite.

The full NSS test suite executes servers and clients, and for that it needs to know the hostname, and also use that hostname inside the certificates it creates. The NSS test suite attempts to automatically detect that from the system's settings, but that often doesn't work, because frequently system's use a hostname that isn't registered in DNS. The hostname localhost.localdomain works everywhere, even on systems that aren't registered in DNS. That's why we commonly set these variables when executing the NSS test suite.

Comment 32

[Kai Engert [:KaiE:]]

+Copy all files from the ./tb subdirectory to the TB tree, e.g.
+cp -v ./tb/* $HOME/your-mozilla-tree/comm/mailnews/test/data/smime/

Perhaps you could insclude a schell script for this and refer to that in the
README.

That's a very good idea!

And I realize that we don't need to clone nspr/nss from the server.
I'll add a generate.sh script. With that approach, I think your script optimizations aren't necessary.

The script will create a subdirectory for building, will copy nspr/nss from the parent mozilla tree, build, run the test, copy all the files, and remove the build tree.

Comment 33

[Kai Engert [:KaiE:]]

You can clean this entire bit up with

async function check_smime_message() {
for (let message of messages)
await check_smime(messages[i]);
}

I think that wouldn't work, because, this implementation was using a cascade. All async tests were started immediately, but the chain of promises assured that e.g. test 2 was delayed until after test 1 was done, and test 3 was delayed until test 2 was done, etc. That's why the waiting needed to be done at the beginning of each test, for the promise of the previous test. I needed to ensure that tests don't run in parallel.

But I realized this cascade isn't necessary, and found a simpler solution.

I reimplemented it to use just two promises:

• one function copies everything, and uses a promise to signal copying is done
• the test function waits for copying to be done, then loops over all the tests, then signals that testing is done
• the cleanup waits for the signal that testing is done

Comment 34

[Kai Engert [:KaiE:]]

Attachment: 1011625-jcranmer-v8.patch

Addressed the minor cleanup requests.

Comment 35

[Kai Engert [:KaiE:]]

Attachment: 1011625-kaie-v8.patch

Comment 36

[Kai Engert [:KaiE:]]

Comment on attachment 9047485 [details] [diff] [review]
1011625-kaie-v8.patch

Alternatively, asking Magnus for review.

Comment 37

[Kai Engert [:KaiE:]]

Magnus, you had given feedback+ on the previous patch (attachment 9046125 [details] [diff] [review]). This version implements the requested changes, and it simplifies the async logic.

Comment 38

[Kai Engert [:KaiE:]]

Attachment: 1011625-test-data.patch (no code)

Raw test data files, as a patch.

Comment 39

[Kai Engert [:KaiE:]]

Attachment: 1011625-jcranmer-v9.patch

same code as before, but properly formatted patch

Comment 40

[Kai Engert [:KaiE:]]

Attachment: 1011625-kaie-v9.patch

Comment 41

[Magnus Melin [:mkmelin]]

Comment on attachment 9048147 [details] [diff] [review]
1011625-kaie-v9.patch

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

On a higher level it strikes me, what should we do to test messages that are not in the expected format. I mean, messages that we can't generate but that are found in the wild....

Looks good to me. r=mkmelin with some nits

::: mailnews/mime/test/unit/test_smime_decrypt.js
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

Please add a file level documentation. These are shown e.g. on dxr to and make it easier to figure out what the file does. Something like

/**
 * Tests for S/MIME ensuring that encrypted and signed messages are shown as such, or shown as invalid when that is the case.
 */

@@ +192,5 @@
> +
> +add_task(async function cleanup() {
> +  await gCleanupWaiter.promise;
> +  //dump("====> cleanup\n");
> +  gInbox = null;

I think this is not necessary. xpcshell tests clean up their environment themselves for each run

::: mailnews/test/data/smime/generate.sh
@@ +1,2 @@
> +#!/bin/bash
> +

You seem to have lost the instructions on where to get the nss build through hg

@@ +19,5 @@
> +cp -v nssbuild/tests_results/security/localhost.1/sharedb/smime/tb/* .
> +
> +echo "Done. Will remove the NSS build/test tree in 20 seconds."
> +sleep 20
> +rm -rf nssbuild

might not want to remove it (in case of errors, or in case someone needs to go back and fiddle with something)

Comment 42

[Magnus Melin [:mkmelin]]

Can you also in the README write down the date for when first expiration is expected.

Comment 43

[Kai Engert [:KaiE:]]

(In reply to Magnus Melin [:mkmelin] from comment #42)

Can you also in the README write down the date for when first expiration is expected.

I'm worried that will get out of sync on next update. It might be better to automatically produce that information, and list it in a separate file.

I should be able to enhance the generate.sh script to list that information.

Comment 44

[Kai Engert [:KaiE:]]

(In reply to Magnus Melin [:mkmelin] from comment #41)

On a higher level it strikes me, what should we do to test messages that are
not in the expected format. I mean, messages that we can't generate but that
are found in the wild....

If we're talking about valid messages, this basic set of messages already contains both variations for signed messages. Although Thunderbird only produces one variation, NSS can be used to produce both variations.

Comment 45

[Kai Engert [:KaiE:]]

(In reply to Magnus Melin [:mkmelin] from comment #41)

You seem to have lost the instructions on where to get the nss build through
hg

Not sure what you're referring to. We no longer have to clone from hg, because the Mozilla tree contains all the NSS files that we need, and we simply make a local copy operation to get them.

The script automates building and generating all files, and copying them to the right place.

+echo "Done. Will remove the NSS build/test tree in 20 seconds."
+sleep 20
+rm -rf nssbuild

might not want to remove it (in case of errors, or in case someone needs to
go back and fiddle with something)

After the script has been executed, the new files obviously must be committed.

If we don't clean up, the person doing the refresh might accidentally commit a very large set of new files.

Comment 46

[Kai Engert [:KaiE:]]

Attachment: 1011625-kaie-v10.patch

Comment 47

[Kai Engert [:KaiE:]]

Attachment: 1011625-jcranmer-v10.patch

Added a file level comment to Joshua's code.

Comment 48

[Kai Engert [:KaiE:]]

Comment on attachment 9048142 [details] [diff] [review]
1011625-test-data.patch (no code)

Magnus, this attachment adds the test data files.

How should they be marked as reviewed for checkin?

Comment 49

[Jorg K (CEST = GMT+2)]

Set checkin-needed and let the friendly sheriff know what you want landed. Or land them yourself with a pretty commit message, that is with - and . ;-) - We can discuss on IRC. Is there a try run that makes sure everything works?

Comment 50

[Kai Engert [:KaiE:]]

Attachment: 1011625-jcranmer-v11.patch

Comment 51

[Kai Engert [:KaiE:]]

try build here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=265a77f6352f17080bb4495447b52c28b9a58551

jcranmer-v11 fixes the ES lint issues, as can be seen here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=37f362a0c11137c9b76b2bab4e5d9e1ea8b05a16

Comment 52

[Jorg K (CEST = GMT+2)]

Comment on attachment 9048142 [details] [diff] [review]
1011625-test-data.patch (no code)

I heard this stuff in produced with some NSS script. @bogus.com is not ideal, but let's do it :-)

Comment 53

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5ebc05985626
test data files (certificates and S/MIME messages). r=jorgk
https://hg.mozilla.org/comm-central/rev/9778f8499a7d
utility code for S/MIME testing. r=kaie
https://hg.mozilla.org/comm-central/rev/5d7419c95b2b
add S/MIME tests, and a helper script to create test certificates. r=mkmelin

Comment 54

[Kai Engert [:KaiE:]]

(In reply to Jorg K (GMT+1) from comment #52)

@bogus.com is not ideal

filed bug 1532384