Created attachment 323060 [details] [review]
Also search for certs with PK11_FindCertsFromEmailAddress when S/MIME encrypting

PK11_FindCertsFromEmailAddress has an O(n²) behaviour, but I believe there won't 
be enough certificates with matching email addresses to cause trouble.
However, the function still looks for all certificates first, and then filters them by email address.

And David, regarding you question about whether the second search for certificates would successed when the first have failed. The second call is necessary because, for reasons unknown to me, Evolution looks for the sender's own certificate using nicknames, which to me happens to be "Imported Certificate".

Also, now the recipients certificates have their signature, usage and validity verified. Maybe this should be moved to another patch?

(In reply to Watson Yuuma Sato from comment #1)
> PK11_FindCertsFromEmailAddress has an O(n²) behaviour, but I believe there
> won't 
> be enough certificates with matching email addresses to cause trouble.
> However, the function still looks for all certificates first, and then
> filters them by email address.

Haha, you wish. No, it's stupider than that. It's O(n²) where n is the total number of certificates available; not the number of *matching* certificates. It uses its O(n²) algorithm to build up a linear list of unique certificates, and only *then* does it iterate over the resulting list calling the FindCertsEmailCallback() function. Yay NSS! :)

This is also noted in https://bugzilla.mozilla.org/1253211 which is the NSS upstream bug covering their part of our bug 736808.

> And David, regarding you question about whether the second search for
> certificates would successed when the first have failed. The second call is
> necessary because, for reasons unknown to me, Evolution looks for the
> sender's own certificate using nicknames, which to me happens to be
> "Imported Certificate".

Yeah, that's because we have a configuration GUI which lets the user *choose* which certificate to use for their own signing/encryption. And we store a reference to that cert by 'nickname'. We should fix that to use the PKCS#11 URI for the object instead of the nickname.

That aside, I think we should handle the sender's own certificate *separately*, looking it up by nickname (and later its RFC7512 URI). Rather than using the email -> nickname fallback for *all* addresses, we should handle each one the *right* way.

It also occurs to me that we're doing this stupid NSS O(n²) thing for *every* address we're sending to. And I thought it was bad to do it just *three* times in the preferences dialog :)

I think we should eschew PK11_FindCerts*() altogether and have our *own* implementation of NSSTrustDomain_TraverseCertificates(). With our *own* function as the initial callback, instead of their stupid collector() function.
And then ∀ cert, we could check if it's one we want for *any* of our recipients, and shove it straight into our receipient_certs[] array.

Or if that requires access to NSS internals and can't easily be done, just call P11_ListCerts() once and then use the results locally.

Furthermore, when NSS does get a PK11_FindCertFromUri() function (see GSoC proposal this year at https://www.mail-archive.com/dev-tech-crypto@lists.mozilla.org/msg12493.html ) we'll want to use that *first* because it can do a fast direct lookup for certs with CKA_SUBJECT matching the email address. And only if that *fails* do we have to go grubbing around in *all* the certs, looking for one which had a less sensible CKA_SUBJECT.

> Also, now the recipients certificates have their signature, usage and
> validity verified. Maybe this should be moved to another patch?

There's merit in that suggestion. That way, if the changes introduce problems, it's easy for someone to try reverting just *that* part to see which made the difference.

(In reply to David Woodhouse from comment #2)
> Yeah, that's because we have a configuration GUI which lets the user
> *choose* which certificate to use for their own signing/encryption. And we
> store a reference to that cert by 'nickname'. We should fix that to use the
> PKCS#11 URI for the object instead of the nickname.

In this case the PKCS#11 URI would specify the token in which the object resides, right?

> That aside, I think we should handle the sender's own certificate
> *separately*, looking it up by nickname (and later its RFC7512 URI). Rather
> than using the email -> nickname fallback for *all* addresses, we should
> handle each one the *right* way.

The sender may choose to encrypt the mail to himself, or not. If he chooses to do so, his address will be the last in the recipients list. I could not find an elegant way to discover if he is also encrypting for him self.
The way it's is added to the list, in Evolution side is using e_source_smime_get_encrypt_to_self() over an ESourceSMime extension, but in EDS side the camel component knows nothing about ESources.

Maybe the userid parameter from smime_context_encrypt_sync() could be used. 

> It also occurs to me that we're doing this stupid NSS O(n²) thing for
> *every* address we're sending to. And I thought it was bad to do it just
> *three* times in the preferences dialog :)

Indeed.

> I think we should eschew PK11_FindCerts*() altogether and have our *own*
> implementation of NSSTrustDomain_TraverseCertificates(). With our *own*
> function as the initial callback, instead of their stupid collector()
> function.
> And then ∀ cert, we could check if it's one we want for *any* of our
> recipients, and shove it straight into our receipient_certs[] array.

Unfortunately NSSTrustDomain_TraverseCertificates() is not exported.

> Or if that requires access to NSS internals and can't easily be done, just
> call P11_ListCerts() once and then use the results locally.

Would it be acceptable to limit the tokens searched to the internal and evolution-pkcs11 until PK11_FindCertFromUri() is available or hash table is implemented in NSSTrustDomain_TraverseCertificates()?

We could use PK11_FindCertsInSlot(), which seems to be O(n), to search only in evolution-pkcs11 slot. And keep the CERT_FindCertByNicknameOrEmailAddr() to search the internal module.
 
> Furthermore, when NSS does get a PK11_FindCertFromUri() function (see GSoC
> proposal this year at
> https://www.mail-archive.com/dev-tech-crypto@lists.mozilla.org/msg12493.html
> ) we'll want to use that *first* because it can do a fast direct lookup for
> certs with CKA_SUBJECT matching the email address. And only if that *fails*
> do we have to go grubbing around in *all* the certs, looking for one which
> had a less sensible CKA_SUBJECT.

(In reply to Watson Yuuma Sato from comment #3) 
> In this case the PKCS#11 URI would specify the token in which the object
> resides, right?

The token *and* the specific certificate therein.

> The sender may choose to encrypt the mail to himself, or not. If he chooses
> to do so, his address will be the last in the recipients list. I could not
> find an elegant way to discover if he is also encrypting for him self. ...
> Maybe the userid parameter from smime_context_encrypt_sync() could be used. 

It's OK to expand the Camel APIs and pass this explicitly, I think.

> Would it be acceptable to limit the tokens searched to the internal and
> evolution-pkcs11 until PK11_FindCertFromUri() is available or hash table is
> implemented in NSSTrustDomain_TraverseCertificates()?

Iterating over the *tokens* isn't the problem, is it?

> We could use PK11_FindCertsInSlot(), which seems to be O(n), to search only
> in evolution-pkcs11 slot. And keep the CERT_FindCertByNicknameOrEmailAddr()
> to search the internal module.

Do you mean PK11_TraverseCertsInSlot()? That's still using the nssPKIObjectCollection, which I think is where the problematic O(n²) behaviour comes from.

As you know, GSoC candidates are supposed to fix a bug or something first, during the application process. I've pointed Varun (who wants to do the URI support) at this O(n²) thing and he's currently working on switching it over to use hash tables :)

Created attachment 323757 [details] [review]
Allows Evolution to search for certificates in other tokens when S/MIME encrypting

Considering that NSSTrustDomain_TraverseCertificates() has been changed to use hash tables...

This patch reduces the multiple calls to NSSTRustDomain_TraverseCertificates() to one, and so the number of certificate collections created. Also, the callback checks each certificate in the collection against every recipient, instead of only one recipient, as done by PK11_FindCertsFromEmailAddress().

But NSS is still fetching all certificates from all tokens into collection before filtering them.

Review of attachment 323757 [details] [review]:

Please correct me if I'm wrong with the below ideas. There are also certain coding style issues, but I'm skipping them for now, because they are not that important here.

::: camel/camel-smime-context.c
@@ +1052,3 @@
+	 * NSS still wants to run the callback over other certs */
+	if (cbparam->found_all) {
+		return SECSuccess;

I would use SECFailure to quickly stop traversing. I know it can have side-effects, but no big deal if you count with it.

@@ +1068,3 @@
+		}
+
+		/* Is the sender referenced by nickname rather than its email address? */

you do not need to check, if the 'found' is TRUE

@@ +1076,3 @@
+		if (!found)
+			i++;
+	} while (i < cbparam->recipients->len && !found);

If I read this code properly, then for each found certificate you always check for all recipients whether the certificate matches, even if the recipient has the certificate already found. That means: a) memory leak (cbparam->recipient_certs[i]); b) unneeded check for the emails.

I think it's exactly this place where the hash table makes sense. The hash table would contain recipients->pdata[i] as keys, and the certificates as values (NULL at the beginning). There is camel_strcase_hash/camel_strcase_equal, which provides case insensitive hash/equal function pair.

You can also replace the previously found certificate (but not leak it), if the newly found is better, for whatever measure of the 'better-nity'. Maybe it's what you wanted to do here too, only your "better" is just "replace with whatever comes last", where I'm not aware of any particular sort order for these certificates provided by the NSS library (but I do not know NSS at all too).

@@ +1087,3 @@
+				cbparam->found_all = FALSE;
+			}
+		}

This 'for' doesn't add to performance much. You can count how many recipients are handled, instead of checking whenever you find one.

@@ +1139,3 @@
+
+	if (PK11_TraverseSlotCerts (find_recipients_certs, &find_recipients_param, NULL) != SECSuccess) {
+		set_nss_error (error, _("Cannot traverse all slots"));

"Failed to traverse certificates" (see above, whether the error is needed at all)

@@ +1149,3 @@
+				g_set_error (
+					error, CAMEL_ERROR, CAMEL_ERROR_GENERIC,
+					_("No valid or appropriate certificate for email protection for '%s' was found"),

"No valid or appropriate certificate found for '%s'", though the previous error message was good too, I do not know why to change it.

Created attachment 324782 [details] [review]
Allows Evolution to search for certificates in other tokens when S/MIME encrypting

Thank you for the review, suggestions were incorporated in the patch. 

(In reply to Milan Crha from comment #6)
> Review of attachment 323757 [details] [review] [review]:
> 
> ::: camel/camel-smime-context.c
> @@ +1052,3 @@
> +	 * NSS still wants to run the callback over other certs */
> +	if (cbparam->found_all) {
> +		return SECSuccess;
> 
> I would use SECFailure to quickly stop traversing. I know it can have
> side-effects, but no big deal if you count with it.

Found out that the values returned by the callback are completely ignored and PK11_TraverseSlotCerts() is hard coded to return SECSuccess.
So I'm completely ignoring the return value and just checking the certificates in find_recipients_param.

> If I read this code properly, then for each found certificate you always
> check for all recipients whether the certificate matches, even if the
> recipient has the certificate already found. That means: a) memory leak
> (cbparam->recipient_certs[i]); b) unneeded check for the emails.
Indeed.

> I think it's exactly this place where the hash table makes sense. The hash
> table would contain recipients->pdata[i] as keys, and the certificates as
> values (NULL at the beginning). There is
> camel_strcase_hash/camel_strcase_equal, which provides case insensitive
> hash/equal function pair.
I went ahead and used the pointers to certificate from recipient_certs as values, this way the recipient_certs array comes out ready for use.

> You can also replace the previously found certificate (but not leak it), if
> the newly found is better, for whatever measure of the 'better-nity'. Maybe
> it's what you wanted to do here too, only your "better" is just "replace
> with whatever comes last", where I'm not aware of any particular sort order
> for these certificates provided by the NSS library (but I do not know NSS at
> all too).

I have no measure of how better a certificate is compared to another, so I'll stick with the first found for a recipient.

Created attachment 324912 [details] [review]
Allows Evolution to search for certificates in other tokens when S/MIME encrypting

Forgot to destroy the hash table...

Review of attachment 324912 [details] [review]:

Thanks for the updated patch. I tried it and I cannot send an encrypted message with it. There are couple issues:
a) one email address can be entered multiple times in the recipients array, then the hash table size doesn't match recipients->len
b) when an account has set a certificate, then it is not addressed by an email address, but by some certificate "name", in my case: "CAcert WoT User's Root CA ID #2"; this one fails to be found with this patch

I wrote my other notes and suggestions into the patch itself.

::: camel/camel-smime-context.c
@@ +1030,3 @@
+typedef struct FindRecipientsCertsEmailArgStr {
+    GHashTable		*recipients_table;
+    CERTCertificate	**recipient_certs;

this one is not needed now

@@ +1052,3 @@
+	/* In case we have found all recipient's certificates and
+	 * NSS still wants to run the callback over other certs */
+	if (cbparam->certs_found == g_hash_table_size (cbparam->recipients_table)) {

having a variable 'certs_missing', instead of 'certs_found', you can call g_hash_table_size() only once.

@@ +1063,3 @@
+				cert_email,
+				NULL,
+				(gpointer) &hash_value);

Please do not do this. Strict aliasing should not be worked around this way (I know, I made similar changes in the past, but it's a long time ago and I know I made it wrong in the past). You even do not need the 'key' part, thus simple hash_value = g_hash_table_lookup () works the same, without the strict aliasing issue.

@@ +1074,3 @@
+				cert->nickname,
+				NULL,
+				(gpointer) &hash_value);

ditto (strict aliasing as above)

I extended the last patch and committed it to sources.

Created commit d4c58194b in eds master (3.29.3+)
Created commit 68f8f666d in eds gnome-3-28 (3.28.3+)

And I overlooked a non-unicode apostrophes in a translatable string, thus also
Created commit 8e5e1481d in eds master (3.29.3+)

*** Bug 783463 has been marked as a duplicate of this bug. ***

Ehm, a little corrections had been made within bug #792610 comment #4 changes.